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

The command ng serve is not responding to changes to ng module #34813

Closed
1 of 15 tasks
wagnermaciel opened this issue Dec 6, 2019 · 12 comments
Closed
1 of 15 tasks

The command ng serve is not responding to changes to ng module #34813

wagnermaciel opened this issue Dec 6, 2019 · 12 comments
Assignees
Labels
area: compiler Issues related to `ngc`, Angular's template compiler freq2: medium type: bug/fix
Milestone

Comments

@wagnermaciel
Copy link
Contributor

🐞 Bug report

Command

  • new
  • build
  • serve
  • test
  • e2e
  • generate
  • add
  • update
  • lint
  • xi18n
  • run
  • config
  • help
  • version
  • doc

Description

ng serve either is not responding to changes made to the declarations property of an NgModule.

🔬 Minimal Reproduction

  1. ng new pokemon --routing=false --style=css
  2. ng generate component pokedex
  3. set up pokedex to be used by the root component
  4. ng serve
  5. remove pokedex from the declarations field of the NgModule in app.module.ts

Expected

The app crashes

Actual

The app continues working

Additional Information

Restarting ng serve fixes the issue.
This also works vice-versa, so if the application is crashing due to pokedex not being in the declarations of app.module.ts, adding it without restarting ng serve will still be crashing.

🌍 Your Environment

image

Demo

I've created a demo project here.
To reproduce this issue

  1. git clone https://github.com/wagnermaciel/Pokedex.git
  2. npm install
  3. ng serve
  4. Go to http://localhost:4200/pikachu
  5. remove PokedexComponent from the declarations of the NgModule in src/app.app.module.ts
@filipesilva
Copy link
Contributor

I think that's actually related to @angular/core and the rest of the framework being at rc.2. I know that there were some incorrect rebuild states in that version. Can you try with the latest RC?

@wagnermaciel
Copy link
Contributor Author

@filipesilva Sorry, I'm still a little new here. Could you clarify what you mean by "the latest RC"? Based on the output from running ng version, I thought that all of my angular env is using rc.5

@filipesilva
Copy link
Contributor

Yes rc.5 is the latest RC, but https://github.com/wagnermaciel/Pokedex/blob/master/package.json was using rc.2 so I just wanted to make sure. Will look into it, thanks!

@wagnermaciel
Copy link
Contributor Author

Ah, got it. I'll take another look when I get the chance. Thank you!

@wagnermaciel
Copy link
Contributor Author

I tried this out again and it seems to be fixed - somewhere between rc.5 and rc.8. There is still some weirdness where deleting a necessary declaration of a component will rebuild successfully (which is the unexpected/incorrect outcome) and then after a couple seconds (correctly) fail, but this is much more minor than the issue I initially observed.

@filipesilva I spoke with @dgp1130 and we concluded that although package.json says rc.2 it seems like my global version is what was getting used - in the above case rc.5 and currently rc.8. I believe this is because of the semver's in the angular version declarations in package.json.

@wagnermaciel
Copy link
Contributor Author

Seems like I was able to reproduce this. I believe this issue is mainly to do with how ng serve interacts with Angular's RouterModule.

I've created as simple an angular app to demo the bug as I could.
https://github.com/wagnermaciel/bug-demo/tree/master

As a recap - The bug happens when a component is undeclared but used through the RouterModule. Incremental builds will not detect adding or removing the component from the NgModule's declarations, but killing and restarting the server will.

Here is my current environment

Screen Shot 2020-01-15 at 12 05 42 PM

cc: @dgp1130

@wagnermaciel wagnermaciel reopened this Jan 15, 2020
@filipesilva
Copy link
Contributor

I think the answer to the PikachuComponents "where are you?" question will likely be found in the Framework repository, as the CLI doesn't do much to affect the result of AOT compilation in this case. Moved it there.

@filipesilva filipesilva transferred this issue from angular/angular-cli Jan 16, 2020
@dgp1130
Copy link
Contributor

dgp1130 commented Jan 16, 2020

@wagnermaciel, wasn't there a way to run ng serve, make a change, and the automated rebuild would succeed, but then a subsequent ng serve would fail? That sounds like a CLI issue. What were the steps to reproduce that problem?

@wagnermaciel
Copy link
Contributor Author

Yes, the issue here is not that the build fails or succeeds. The issue is that an iterative rebuild can look like everything is working just fine but killing and restarting the server will show that the app is actually crashing, or vice-versa.

@alxhub alxhub self-assigned this Jan 16, 2020
@alxhub alxhub added this to the v9-candidates milestone Jan 16, 2020
@ngbot ngbot bot removed this from the v9-candidates milestone Jan 16, 2020
@alxhub alxhub added area: compiler Issues related to `ngc`, Angular's template compiler freq2: medium severity3: broken type: bug/fix labels Jan 16, 2020
@ngbot ngbot bot modified the milestone: Backlog Jan 16, 2020
@alxhub
Copy link
Member

alxhub commented Jan 16, 2020

Confirmed as reported in rc.9.

There are actually two bugs here!

  1. it's not a compiler error that PokedexComponent uses a pipe that's not defined (if it's not in the AppModule, it doesn't have AsyncPipe in its scope). This should be caught by template type-checking. I'll need to investigate this.

  2. the rebuild does indeed not catch that PokedexComponent needs to be rebuilt.

@alxhub
Copy link
Member

alxhub commented Jan 22, 2020

I've reproduced this in the compiler's incremental build tests now.

The general flow of the bug is:

  1. We detect that the component's previous record isn't reusable, as the component's file previously depended on the module's file, which has changed. So far, so good.

  2. We successfully analyze/resolve the component in its new context, with no associated module. Also as expected.

  3. We close out the analysis phase by reporting success to the incremental build subsystem, which transitions from a "pending" to an "analyzed" state. This involves using the newly completed dependency graph (for the current program) to determine which files need to actually be emitted.

Here's where things go off the rails. If the component had been moved from one module to another, it would have a dependency on its new module's file, which would have changed (to add the component). This would trigger an emit of the component.

But because the component now has no associated module, it now has no outgoing dependency edges to anything that's changed. Therefore, the incremental build engine decides that the component isn't stale after all, and does not require a re-emit.


What's missing from this picture is the understanding that not depending on a module is, in fact, a change. Somehow, we need to carry forward the information that the component file still depends on the module file in the new program, because the removal of the component from the module is still a change which affects the component.

I'll have to think about how to express this cleanly.

alxhub added a commit to alxhub/angular that referenced this issue Jan 22, 2020
This commit fixes a bug in the incremental rebuild engine of ngtsc, where if
a component was removed from its NgModule, it would not be properly
re-emitted.

The bug stemmed from the fact that whether to emit a file was a decision
based purely on the updated dependency graph, which captures the dependency
structure of the rebuild program. This graph has no edge from the component
to its former module (as it was removed, of course), so the compiler
erroneously decides not to emit the component.

The bug here is that the compiler does know, from the previous dependency
graph, that the component file has logically changed, since its previous
dependency (the module file) has changed. This information was not carried
forward into the set of files which need to be emitted, because it was
assumed that the updated dependency graph was a more accurate source of that
information.

With this commit, the set of files which need emit is pre-populated with the
set of logically changed files, to cover edge cases like this.

Fixes angular#34813
alxhub added a commit to alxhub/angular that referenced this issue Jan 22, 2020
This commit fixes a bug in the incremental rebuild engine of ngtsc, where if
a component was removed from its NgModule, it would not be properly
re-emitted.

The bug stemmed from the fact that whether to emit a file was a decision
based purely on the updated dependency graph, which captures the dependency
structure of the rebuild program. This graph has no edge from the component
to its former module (as it was removed, of course), so the compiler
erroneously decides not to emit the component.

The bug here is that the compiler does know, from the previous dependency
graph, that the component file has logically changed, since its previous
dependency (the module file) has changed. This information was not carried
forward into the set of files which need to be emitted, because it was
assumed that the updated dependency graph was a more accurate source of that
information.

With this commit, the set of files which need emit is pre-populated with the
set of logically changed files, to cover edge cases like this.

Fixes angular#34813
alxhub added a commit to alxhub/angular that referenced this issue Jan 23, 2020
This commit fixes a bug in the incremental rebuild engine of ngtsc, where if
a component was removed from its NgModule, it would not be properly
re-emitted.

The bug stemmed from the fact that whether to emit a file was a decision
based purely on the updated dependency graph, which captures the dependency
structure of the rebuild program. This graph has no edge from the component
to its former module (as it was removed, of course), so the compiler
erroneously decides not to emit the component.

The bug here is that the compiler does know, from the previous dependency
graph, that the component file has logically changed, since its previous
dependency (the module file) has changed. This information was not carried
forward into the set of files which need to be emitted, because it was
assumed that the updated dependency graph was a more accurate source of that
information.

With this commit, the set of files which need emit is pre-populated with the
set of logically changed files, to cover edge cases like this.

Fixes angular#34813
AndrewKushnir pushed a commit that referenced this issue Jan 23, 2020
…34912)

This commit fixes a bug in the incremental rebuild engine of ngtsc, where if
a component was removed from its NgModule, it would not be properly
re-emitted.

The bug stemmed from the fact that whether to emit a file was a decision
based purely on the updated dependency graph, which captures the dependency
structure of the rebuild program. This graph has no edge from the component
to its former module (as it was removed, of course), so the compiler
erroneously decides not to emit the component.

The bug here is that the compiler does know, from the previous dependency
graph, that the component file has logically changed, since its previous
dependency (the module file) has changed. This information was not carried
forward into the set of files which need to be emitted, because it was
assumed that the updated dependency graph was a more accurate source of that
information.

With this commit, the set of files which need emit is pre-populated with the
set of logically changed files, to cover edge cases like this.

Fixes #34813

PR Close #34912
sonukapoor pushed a commit to sonukapoor/angular that referenced this issue Feb 13, 2020
…ngular#34912)

This commit fixes a bug in the incremental rebuild engine of ngtsc, where if
a component was removed from its NgModule, it would not be properly
re-emitted.

The bug stemmed from the fact that whether to emit a file was a decision
based purely on the updated dependency graph, which captures the dependency
structure of the rebuild program. This graph has no edge from the component
to its former module (as it was removed, of course), so the compiler
erroneously decides not to emit the component.

The bug here is that the compiler does know, from the previous dependency
graph, that the component file has logically changed, since its previous
dependency (the module file) has changed. This information was not carried
forward into the set of files which need to be emitted, because it was
assumed that the updated dependency graph was a more accurate source of that
information.

With this commit, the set of files which need emit is pre-populated with the
set of logically changed files, to cover edge cases like this.

Fixes angular#34813

PR Close angular#34912
@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 Feb 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: compiler Issues related to `ngc`, Angular's template compiler freq2: medium type: bug/fix
Projects
None yet
Development

No branches or pull requests

4 participants