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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

ivy loses query results in abstract @Directive() when CLI rebuilt (file change) #32416

Closed
elvisbegovic opened this issue Aug 30, 2019 · 8 comments
Closed

Comments

@elvisbegovic
Copy link
Contributor

@elvisbegovic elvisbegovic commented Aug 30, 2019

馃悶 Bug report

Command

- [x] serve

Description

In IVY v9 (see here), we'll need to add abstract @Directive() on abstracts classes we extends that contain Angular stuff (like Input, ViewChild, DI).
After add @Directive() on my abstract class and run ng serve everything works well.
But If I edit abstract directive file and save, browser autoreload but query results are not resolved anymore ?!?!

馃敩 Minimal Reproduction

One line repro :
git clone https://github.com/elvisbegovic/issue-ivy-abstract-directive.git && cd issue-ivy-abstract-directive && npm i && ng s

Open browser localhost:4200, open devtools console, you will see consoel without error, viewchild is available.

Edit or comment anything in any-abstract-class.ts file (cli rebuild + autoreload browser) you can see error at runtime that viewchild this.myList is undefined.

GIF:
ivyrebuild

馃敟 Exception or Error

After browser auto reload :


ERROR TypeError: Cannot read property 'name' of undefined
    at AppComponent.ngOnInit (any-abstract-class.ts:11)
    at AppComponent.ngOnInit (app.component.ts:17)
    at callHook (core.js:7864)
    at callHooks (core.js:7828)
    at executeInitAndCheckHooks (core.js:7769)
    at refreshView (core.js:12199)
    at renderComponentOrTemplate (core.js:12288)
    at tickRootContext (core.js:13648)
    at detectChangesInRootView (core.js:13683)
    at RootViewRef.detectChanges (core.js:15205)

In CLI console :
ERROR in src/app/app.component.ts(8,5): error TS8001: 'my-list' is not a valid HTML element.

馃實 Your Environment

See environment

Angular CLI: 9.0.0-next.2
Node: 12.9.1
OS: win32 x64
Angular: 9.0.0-next.4
... animations, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router

Package Version

@angular-devkit/architect 0.900.0-next.2
@angular-devkit/build-angular 0.900.0-next.2
@angular-devkit/build-optimizer 0.900.0-next.2
@angular-devkit/build-webpack 0.900.0-next.2
@angular-devkit/core 9.0.0-next.2
@angular-devkit/schematics 9.0.0-next.2
@angular/cli 9.0.0-next.2
@ngtools/webpack 9.0.0-next.2
@schematics/angular 9.0.0-next.2
@schematics/update 0.900.0-next.2
rxjs 6.4.0
typescript 3.5.3
webpack 4.39.3

Anything else relevant?
Problem is only when you edit any-abstract-class.ts file, if after first build you edit other file = no error.

If I add selector to Directive and add it to NgModule declarations[], all works as before.

This is working in View Engine (jit and aot and without decorating abstract classes)

Maybe related to #31560 and all referenced issues

Maybe this can live on angular/angular ?

@petebacondarwin

This comment has been minimized.

Copy link
Member

@petebacondarwin petebacondarwin commented Aug 30, 2019

I think this is a problem with ngtsc incremental compilation?

@petebacondarwin petebacondarwin transferred this issue from angular/angular-cli Aug 30, 2019
@ngbot ngbot bot modified the milestone: needsTriage Aug 30, 2019
@petebacondarwin

This comment has been minimized.

Copy link
Member

@petebacondarwin petebacondarwin commented Aug 30, 2019

I think the solution would be for ngtsc to include all ancestors of components as dependencies that would trigger a rebuild of that component.

@alxhub

This comment has been minimized.

Copy link
Contributor

@alxhub alxhub commented Sep 5, 2019

This shouldn't need to be the case (for locality) but probably should be because of type-checking.

@ngbot ngbot bot modified the milestones: needsTriage, Backlog Sep 5, 2019
@alxhub alxhub added this to Backlog in Compiler via automation Sep 5, 2019
@elvisbegovic

This comment has been minimized.

Copy link
Contributor Author

@elvisbegovic elvisbegovic commented Oct 2, 2019

Will this be solved for 9.0 final?

@petebacondarwin

This comment has been minimized.

Copy link
Member

@petebacondarwin petebacondarwin commented Oct 3, 2019

I think it should. We just need someone with time to look into it. :-/

@brgrz

This comment has been minimized.

Copy link

@brgrz brgrz commented Oct 20, 2019

This also happens when no Directives are involved (only components). I have a setup of AppComponent and LayoutComponent and only included the LayoutComponent within the AppComponent markup and the project runs fine immediately after ng serve then breaks down (meaning noth in LayoutComponent gets rendered) after the first CSS/Less/HTML change in any of the two components.

Thankfully setting enableIvy to false solves this.

@JoostK

This comment has been minimized.

Copy link
Member

@JoostK JoostK commented Oct 30, 2019

Here's a breakdown of the issue:

Abstract directives are not registered with an NgModule, so if the directive is updated the NgModule is not invalidated. The component subclass however is correctly tracked as being dependent on its parent. During incremental state reconciliation, this means that the change to the abstract directive has this effect:

  1. Metadata for the abstract directive is dropped, as the directive has changed.
  2. Metadata for the component is dropped, as one of its dependencies has changed.
  3. Metadata for the NgModule is kept, as no dependent file was changed.

During the subsequent compilation, the NgModule is considered to be safe to skip, which means that none of its metadata is analyzed and registered with LocalModuleScopeRegistry. Basically, this code is not executed:

registerNgModuleMetadata(data: NgModuleMeta): void {
this.assertCollecting();
this.moduleToRef.set(data.ref.node, data.ref);
for (const decl of data.declarations) {
this.declarationToModule.set(decl.node, data.ref.node);
}
}

Notice how this is responsible for the knowledge of which declarations correspond with which NgModule.

The abstract directive and component are reanalyzed, because their source file is correctly deemed unsafe to skip analyzing. Consequently, the component requests its module scope:

  1. the LocalModuleScopeRegistry does not have the required information, as the NgModule was not analyzed so no metadata is present locally.
  2. the IncrementalState.getScopeForComponent is considered, however the scope info that was set aside during the first compilation here:

if (scope !== null) {
this.componentScopeRegistry.registerComponentScope(clazz, scope);
}

is no longer available, as the component resides in a file for which no metadata was copied over during incremental state reconciliation as its metadata was deemed outdated (because of the dependency on the abstract directive).

@JoostK JoostK self-assigned this Oct 31, 2019
JoostK added a commit to JoostK/angular that referenced this issue Oct 31, 2019
During incremental compilations, ngtsc needs to know which metadata
from a previous compilation can be reused, versus which metadata has to
be recomputed as some dependency was updated. Changes to
directives/components should cause the NgModule in which they are
declared to be recompiled, as the NgModule's compilation is dependent
on its directives/components.

When a dependent source file of a directive/component is updated,
however, a more subtle dependency should also cause to NgModule's source
file to be invalidated. During the reconciliation of state from a
previous compilation into the new program, the component's source file
is invalidated because one of its dependency has changed, ergo the
NgModule needs to be invalidated as well. Up until now, this implicit
dependency was not imposed on the NgModule. Additionally, any change to
a dependent file may influence the module scope to change, so all
components within the module must be invalidated as well.

Fixes angular#32416
JoostK added a commit to JoostK/angular that referenced this issue Oct 31, 2019
During incremental compilations, ngtsc needs to know which metadata
from a previous compilation can be reused, versus which metadata has to
be recomputed as some dependency was updated. Changes to
directives/components should cause the NgModule in which they are
declared to be recompiled, as the NgModule's compilation is dependent
on its directives/components.

When a dependent source file of a directive/component is updated,
however, a more subtle dependency should also cause to NgModule's source
file to be invalidated. During the reconciliation of state from a
previous compilation into the new program, the component's source file
is invalidated because one of its dependency has changed, ergo the
NgModule needs to be invalidated as well. Up until now, this implicit
dependency was not imposed on the NgModule. Additionally, any change to
a dependent file may influence the module scope to change, so all
components within the module must be invalidated as well.

This commit fixes the bug by introducing additional file dependencies,
as to ensure a proper rebuild of the module scope and its components.

Fixes angular#32416
@JoostK JoostK added the state: has PR label Nov 2, 2019
kara added a commit that referenced this issue Nov 12, 2019
During incremental compilations, ngtsc needs to know which metadata
from a previous compilation can be reused, versus which metadata has to
be recomputed as some dependency was updated. Changes to
directives/components should cause the NgModule in which they are
declared to be recompiled, as the NgModule's compilation is dependent
on its directives/components.

When a dependent source file of a directive/component is updated,
however, a more subtle dependency should also cause to NgModule's source
file to be invalidated. During the reconciliation of state from a
previous compilation into the new program, the component's source file
is invalidated because one of its dependency has changed, ergo the
NgModule needs to be invalidated as well. Up until now, this implicit
dependency was not imposed on the NgModule. Additionally, any change to
a dependent file may influence the module scope to change, so all
components within the module must be invalidated as well.

This commit fixes the bug by introducing additional file dependencies,
as to ensure a proper rebuild of the module scope and its components.

Fixes #32416

PR Close #33522
@kara kara closed this in 15f8638 Nov 12, 2019
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Dec 13, 2019

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 Dec 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can鈥檛 perform that action at this time.