Skip to content
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): revert change to component dir in generate module #3158

Merged
merged 1 commit into from Nov 17, 2016

Conversation

Meligy
Copy link
Contributor

@Meligy Meligy commented Nov 16, 2016

fix(generate): revert change to component dir in generate module, as it caused component declaration to go to parent module

This fixes regression introduced in the code-review of PR #3066. The PR deals with generate module command, and the component generated with the module.

In the code review, it was requested to change the way component was created as flat. I did that, but this is causing the following problem:

If you run:

ng new cli-project
cd cli-project
ng g module test

The files are created in src/app/test correctly (and hence the tests pass), but the component src/app/test/test.component.ts is added to the NgModule declarations of AppModule, not TestModule.

This is because with switching flat false and giving the module folder as the component folder, the CLI looks for a module to add declarations ABOVE the module folder.

So, now I reverted the code to the original code in #3066 before the code review.

@Meligy
Copy link
Contributor Author

Meligy commented Nov 16, 2016

@Brocco @hansl I think this issue is quite severe (basically ng generate module is broken), so, please kindly give this PR high priority in reviewing, merging, and releasing.

@Meligy
Copy link
Contributor Author

Meligy commented Nov 16, 2016

Adding @filipesilva since @Brocco is traveling and may not have time to check this.

This is important because basically ng generate module is broken in the latest release (Adds component to the declarations of the wrong module).

Copy link
Contributor

@filipesilva filipesilva left a comment

Choose a reason for hiding this comment

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

@Meligy can you add tests to show this worked as you expected? You can use the scenario you mentioned as generating the wrong output, and verify the import is in the right file via a regex in expectFileToMatch.

@Meligy Meligy force-pushed the fix/generate-router-declaration branch from 99a9002 to 0471178 Compare November 16, 2016 23:38
…it caused component declaration to go to parent module
@Meligy Meligy force-pushed the fix/generate-router-declaration branch from 0471178 to b679089 Compare November 16, 2016 23:40
Copy link
Contributor Author

@Meligy Meligy left a comment

Choose a reason for hiding this comment

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

@filipesilva test updated, CI passed.

@filipesilva
Copy link
Contributor

Thank you for fixing this @Meligy!

@filipesilva filipesilva merged commit 71bf855 into angular:master Nov 17, 2016
@Meligy
Copy link
Contributor Author

Meligy commented Nov 17, 2016

@filipesilva you are super awesome man.

Thanks a lot for the merge, and especially for the testing guidance showing the goodness with e2e tests.

Can you please release beta.20-5 or beta.21 with this fix? Again, I really wouldn't be that pushy if it weren't an issue with a common command and released already.

Cheers,

@filipesilva
Copy link
Contributor

We should have a release soon, that'll include this and some performance fixes.

@Meligy
Copy link
Contributor Author

Meligy commented Nov 17, 2016

Brilliant!

Just saw another fairly critical issues in Angular 2.2.0 itself too angular/angular#12911.

As per #3175 (comment), there should be a 2.2.1 release with the fix soon. Might be a good idea to wait to include that in the next beta as well.

@Meligy
Copy link
Contributor Author

Meligy commented Nov 17, 2016

Actually 2.2.1 is out already https://github.com/angular/angular/blob/master/CHANGELOG.md. That would be good to have in next release, although the NPM version check should match it anyway.

Mischi pushed a commit to BROCKHAUS-AG/angular-cli that referenced this pull request Nov 19, 2016
…it caused component declaration to go to parent module (angular#3158)
MRHarrison pushed a commit to MRHarrison/angular-cli that referenced this pull request Feb 9, 2017
…it caused component declaration to go to parent module (angular#3158)
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants