Skip to content
This repository was archived by the owner on Apr 4, 2025. It is now read-only.

fix(@schematics/angular): Allow for scoped library names #646

Merged
merged 2 commits into from
Apr 25, 2018

Conversation

Brocco
Copy link
Contributor

@Brocco Brocco commented Apr 6, 2018

const name = options.name;
// If scoped project (i.e. "@foo/bar"), convert projectDir to "foo-bar".
const packageName = options.name;
options.name = /@[\S]*\/[\S]*/.test(options.name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the name passed on the command line is a/b/c? Is it going to create c in /projects/a/b?

@hansl
Copy link
Contributor

hansl commented Apr 6, 2018

The removal of @ and replacing / with - looks good, I just want to make sure we still allow for other cases.

@filipesilva
Copy link
Contributor

I think it would be good to keep the /. So @scoped/lib would go into projects/scoped/lib, kinda like we have things in the CLI and DevKit repos. No opinion on the @ myself.

@Brocco
Copy link
Contributor Author

Brocco commented Apr 10, 2018

@about-code in my experience having @ in paths messes up autocompleting paths in consoles (both Mac & Windows) which is why I removed it as part of the directory structure.

let scopeName = '';
if (/@[\S]*\/[\S]*/.test(options.name)) {
const [scope, name] = options.name.split('/');
scopeName = scope.replace('@', '');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace(/^@/, '')

@hansl
Copy link
Contributor

hansl commented Apr 10, 2018

Question; do we check that I cannot enter any strings somewhere? Do we have a pattern in the schema?

@Brocco
Copy link
Contributor Author

Brocco commented Apr 11, 2018

@hansl I updated the logic to validate the project name... but not via a schema pattern in order to provide more verbose error messages.

@Brocco Brocco force-pushed the scoped-library branch 2 times, most recently from 2f16d7a to 6996de1 Compare April 12, 2018 02:46
@about-code
Copy link

I am not yet sure whether it's preferable to strip the @ even though I see the advantages on the console. I certainly don't know enough about the full library implementation in the CLI to make any concrete suggestions. But let me explain some thoughts I had, e.g. about how it might affect webpack module resolution and potential benefits for lazy loaded modules:

Assume an Angular app wants to lazy load an NgModule from a library in the projects folder. If Webpack were configured to resolve import paths from the projects folder like so:

module: { 
     resolve: ["./projects", "./node_modules"]
}

then it might (not tested in CLI context) become possible to compile angular apps which load an NgModule from a library with an absolute path beginning with the package name maybe like so:

// Assumption: AoT-Compiler translates the path to a dynamic webpack import()
loadChildren: '@foo/bar/src/lib/bar.module#BarModule' 

Webpack were able to resolve import("@foo/bar") by not just looking into node_modules but also in projects which may avoid the need for npm linking, or copying things into node_modules in some situations. I am not sure how CLI projects are configured in this regard but I think it could have some benefits to take npm module resolution algorithms into account.

@filipesilva
Copy link
Contributor

@about-code we add TS path configuration pointing the to built library artifacts. Our webpack plugin adds the TS paths to the module resolution logic of webpack. This way the folder name proper does not matter.

@about-code
Copy link

@filipesilva Thank you very much for your response and for taking away my concerns. Now I am gonna be quiet on this. 😃

@Brocco Brocco force-pushed the scoped-library branch 4 times, most recently from d218225 to 693ec98 Compare April 23, 2018 21:41
} else if (unsupportedProjectNames.indexOf(projectName) !== -1) {
throw new SchematicsException(`Project name "${projectName}" is not a supported name.`);
} else if (!packageNameRegex.test(projectName)) {
throw new SchematicsException(`Project name (${projectName}) is invalid.`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

${JSON.stringify(projectName)}

hansl
hansl previously approved these changes Apr 24, 2018
hansl
hansl previously approved these changes Apr 25, 2018
@rodchenkov
Copy link

Hi All
Just found that in tsconfig.json path name for scoped library is without scope name.
I think we need to solve this.

@filipesilva
Copy link
Contributor

@rodchenkov I have a fix incoming in #895

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to generate a scoped library with v6 rc0
6 participants