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(ivy): reuse compilation scope for incremental template changes. #31932

Conversation

@petebacondarwin
Copy link
Member

commented Jul 31, 2019

Previously if only a component template changed then we would know to
rebuild its component source file. But the compilation was incorrect if the
component was part of an NgModule, since we were not capturing the
compilation scope information that had a been acquired from the NgModule
and was not being regenerated since we were not needing to recompile
the NgModule.

Now we register compilation scope information for each component, via the
ComponentScopeRegistry interface, so that it is available for incremental
compilation.

The ComponentDecoratorHandler now reads the compilation scope from a
ComponentScopeReader interface which is implemented as a compound
reader composed of the original LocalModuleScopeRegistry and the
IncrementalState.

Fixes #31654

@petebacondarwin petebacondarwin requested a review from angular/fw-compiler as a code owner Jul 31, 2019

@ngbot ngbot bot added this to the needsTriage milestone Jul 31, 2019

@googlebot googlebot added the cla: yes label Jul 31, 2019

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngtsc-incremental-resource-fix-issue-15122-a branch from 277e9d6 to 96062e9 Jul 31, 2019

@petebacondarwin petebacondarwin requested a review from angular/fw-ngcc as a code owner Jul 31, 2019

@gkalpak
Copy link
Member

left a comment

LGTM for fw-ngcc.

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngtsc-incremental-resource-fix-issue-15122-a branch from 96062e9 to 2dc3172 Jul 31, 2019

@alxhub

alxhub approved these changes Aug 5, 2019

}

getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null {
const metadata = this.metadata.get(clazz.getSourceFile()) || null;

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 5, 2019

Contributor

Use .has for map key presence and not || null

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Aug 6, 2019

Author Member

Honestly, I think that this results in more verbose, ugly and less clear code:

  getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null {
    const metadata = this.metadata.has(clazz.getSourceFile()) ?
        this.metadata.get(clazz.getSourceFile()) ! :
        null;
    return metadata && metadata.componentScope.has(clazz) ? metadata.componentScope.get(clazz) ! :
                                                            null;
  }

Notice the repetition and the need to use the ! operator.

I think it is fair to use has() when the type of the stored object could be falsy. But where a falsy return from get() can only mean the value was not there, then I think it is better to avoid the extra call to has().

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Aug 6, 2019

Author Member

Slightly cleaner version, which I can accept:

  getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null {
    if (!this.metadata.has(clazz.getSourceFile())) {
      return null;
    }
    const metadata = this.metadata.get(clazz.getSourceFile()) !;
    if (!metadata.componentScope.has(clazz)) {
      return null;
    }
    return metadata.componentScope.get(clazz) !;
  }

This comment has been minimized.

Copy link
@gkalpak

gkalpak Aug 6, 2019

Member

I live for the day .has() will narrow the type of a subsequent .get() 😁

This comment has been minimized.

Copy link
@alfaproject

alfaproject Aug 6, 2019

Contributor

OOC wouldn't doing just a get be more performant anyway if you need the value? I see has for the use case of just testing whether or not a key exists without needing the actual value.

metadata.remoteScoping.add(clazz);
}

getRequiresRemoteScope(clazz: ClassDeclaration): boolean|null {

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 5, 2019

Contributor

Can you file a JIRA and add a TODO here...

Remote scoping is gonna be tricky. If a component requires remote scoping, it means that the component's template changing requires the module to be re-emitted. Also, we need to make sure the cycle detector works well across rebuilds. Incremental compilation is ugly.

This comment has been minimized.

fix(ivy): reuse compilation scope for incremental template changes.
Previously if only a component template changed then we would know to
rebuild its component source file. But the compilation was incorrect if the
component was part of an NgModule, since we were not capturing the
compilation scope information that had a been acquired from the NgModule
and was not being regenerated since we were not needing to recompile
the NgModule.

Now we register compilation scope information for each component, via the
`ComponentScopeRegistry` interface, so that it is available for incremental
compilation.

The `ComponentDecoratorHandler` now reads the compilation scope from a
`ComponentScopeReader` interface which is implemented as a compound
reader composed of the original `LocalModuleScopeRegistry` and the
`IncrementalState`.

Fixes #31654

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngtsc-incremental-resource-fix-issue-15122-a branch from 55ae1cf to d643043 Aug 6, 2019

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

@kara kara closed this in eb5412d Aug 9, 2019

@petebacondarwin petebacondarwin deleted the petebacondarwin:ngtsc-incremental-resource-fix-issue-15122-a branch Aug 11, 2019

pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this pull request Aug 12, 2019

fix(ivy): reuse compilation scope for incremental template changes. (a…
…ngular#31932)

Previously if only a component template changed then we would know to
rebuild its component source file. But the compilation was incorrect if the
component was part of an NgModule, since we were not capturing the
compilation scope information that had a been acquired from the NgModule
and was not being regenerated since we were not needing to recompile
the NgModule.

Now we register compilation scope information for each component, via the
`ComponentScopeRegistry` interface, so that it is available for incremental
compilation.

The `ComponentDecoratorHandler` now reads the compilation scope from a
`ComponentScopeReader` interface which is implemented as a compound
reader composed of the original `LocalModuleScopeRegistry` and the
`IncrementalState`.

Fixes angular#31654

PR Close angular#31932
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.