-
Notifications
You must be signed in to change notification settings - Fork 12k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(generate): correct component path when module is generated in subfolder, and parent folder is not a module too #3916
fix(generate): correct component path when module is generated in subfolder, and parent folder is not a module too #3916
Conversation
e9da693
to
b4e3266
Compare
b4e3266
to
4369fd2
Compare
Could you also add an e2e test? Acceptance tests are good but likely to be removed after 1.0 but e2e will remain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, all that's missing would be an additional e2e test, as noted above.
4369fd2
to
4bd2bc6
Compare
@hansl Added e2e test |
00e79c5
to
ccfaf9d
Compare
e2e test is passing now. |
LGTM. Need @Brocco's approval. |
// as `this.dynamicPath.dir` will be the same as `this.dynamicPath.appRoot` | ||
// 2. If it does have `/` (like `parent/mod-name`), it'll be `/parent/mod-name/mod-name` | ||
// as `this.dynamicPath.dir` minus `this.dynamicPath.appRoot` will be `/parent` | ||
var dmouleDir = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo? rename please.
dmouleDir
-> moduleDir
Also, please rebase and then it can be merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
…folder, and parent folder is not a module too fixes angular#3255
ccfaf9d
to
512d53a
Compare
Can we mark this for merge now? Cheers, |
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
…folder, and parent folder is not a module too (angular#3916) fixes angular#3255
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
fixes #3255
Another attempt at using the correct component name / path when generating modules, that takes care of the special case in #3255.
Existing behaviour that this PR fixes
if you
ng g module parent --routing
, you canng g module parent/child --routing
.But if
src/app/parent
is just a normal folder, the component generation forng g module parent/child
fails.To test the PR:
ng g module normal-case --routing
to ensure basic scenario is not brokenng g module parent
andng g module parent/child --routing
to ensure this scenario is not broken toomkdir src/app/parent-folder
, and thenng g module parent-folder/child-module --routing
to test that this PR does what it promises to doOr alternatively look at the provided tests in the PR and existing tests, and ensure they cover all above scenarios, and are passing in CI.
Note
This is maybe the 3rd or so attempt at getting this path to work in all scenarios. We are protected by tests though, and we keep adding more tests the more scenarios we get. Let's see if this becomes the last change of this path/name.