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
Update to NgModule Registration #45024
Conversation
4784321
to
4a4f86c
Compare
e0225c6
to
30a5b7a
Compare
30a5b7a
to
a83e728
Compare
a83e728
to
e1c2282
Compare
Marking this as blocked since the partial compilation tests need a bit of work still. |
// insert statements after definitions. To work around this, the linker transforms the | ||
// definition into an IIFE which executes the definition statements before returning the | ||
// definition expression. | ||
const importGenerator = new LinkerImportGenerator(this.ngImport); |
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.
Consider pulling this up because it's also used earlier
packages/compiler-cli/linker/src/file_linker/emit_scopes/local_emit_scope.ts
Outdated
Show resolved
Hide resolved
e1c2282
to
f3509ff
Compare
759ada1
to
b4c34d9
Compare
Note to the Caretaker: this PR will require an additional change in g3, please include this change into the sync. |
The `compileNgModule` operation previously supported a flag `emitInline`, which controlled whether template scoping information for the NgModule was emitted directly into the compiled NgModule definition, or whether an associated statement was generated which patched the information onto the NgModule definition. Both options are useful in different contexts. This commit changes this flag to an enum (and renames it), which allows for a third option - do not emit any template scoping information. This option is added to better represent the actual behavior of the Angular Linker, which sometimes configures `compileNgModule` to use the side-effectful statement generation but which does not actually emit such associated statements. In other words, the linker effectively does not generate scoping information for NgModules at all (in some configurations) and this option more directly expresses that behavior. This is a refactoring as no generated code is changed as a result of introducing this flag, due to the linker's behavior of not emitting associated statements.
When `@angular/compiler` processes metadata and compiles a definition field, it might also choose to return statements that are associated with that definition, and should be included after the type being compiled. Currently, the linker ignores these statements, as there are none generated that are relevant in the linking operation. A challenge to supporting such associated statements is that the linker operates on "declare" expressions, and replaces those expressions with other expressions. It does not have the capability to append statements after the whole type. The linker actually faces this challenge with statements from the `ConstantPool` as well, and solves this problem by generating an IIFE expression that executes the statements and then returns the definition expression. Previously, an `EmitScope` processed the definition and converted it to an expression, as well as collected constant statements from a `ConstantPool`. A special `IifeEmitScope` implementation was used when emitting into a context where top-level constant statements couldn't be added at all, and uses the IIFE strategy in this case. This commit adds blanket support for associated statements to the linker using this IIFE strategy. The main `EmitScope` now uses the IIFE strategy to emit associated statements, and `IifeEmitScope` has been renamed to `LocalEmitScope`. Now, the `LocalEmitScope` represents constant statements as associated statements to the main `EmitScope` implementation, so they get included in the IIFE as well. Tests are adjusted/added to cover this new behavior. This is a refactoring commit because no live generated code is affected - there are no cases where associated statements are present in linked definitions today.
56f13cd
to
d44c839
Compare
@alxhub FYI the CI should be fixed in 4978d0d, the TGP is fully "green" (with an extra change that should be included into the sync). The PR should be ready for merge now. |
… ids Angular contains an NgModule registry, which allows a user to declare NgModules with string ids and retrieve them via those ids, using the `getNgModuleById` API. Previously, we attempted to structure this registration in a clever fashion to allow for tree-shaking of registered NgModules (that is, those with ids). This sort of worked due to the accidental alignment of behaviors from the different tree-shakers involved. However, this trick relies on the generation of `.ngfactory` files and how they're specifically processed in various bundling scenarios. We intend to remove `.ngfactory` files, hence we can no longer rely on them in this way. The correct solution here is to recognize that `@NgModule({id})` is inherently declaring a global side-effect, and such classes should not really be eligible for tree-shaking in the first place. This commit removes all the old registration machinery, and standardizes on generating a side- effectful call to `registerNgModuleType` for NgModules that have ids. There is some risk here that NgModules with unnecessary `id`s may not tree-shake as a result of this change, whereas they would have in previous circumstances. The fix here should be to remove the `id` if it's not needed. Specifying an `id` is a request that the NgModule be retained regardless of any other references, in case it is later looked up by string id.
Previously, the `TraitCompiler` would naively consider a compilation as failed if either analysis or resolution produced any diagnostics. This commit adjusts the logic to only consider error diagnostics, which allows warnings to be produced from `DecoratorHandler`s. This is a precursor commit to introducing such a warning. As such, the logic here will be tested in the next commit.
In early versions of Angular, it was sometimes necessary to provide a `moduleId` to `@Component` metadata, and the common pattern for doing this was to set `moduleId: module.id`. This relied on the bundler to fill in a value for `module.id`. However, due to the superficial similarity between `Component.moduleId` and `NgModule.id`, many users ended up setting `id: module.id` in their NgModules. This is an anti-pattern that has a few negative effects, including preventing the NgModule from tree-shaking properly. This commit changes the compiler to ignore `id: module.id` in NgModules, and instead provide a warning which suggests removing the line entirely.
4978d0d
to
1b90775
Compare
This PR was merged into the repository by commit 8155428. |
…ld (#45024) When `@angular/compiler` processes metadata and compiles a definition field, it might also choose to return statements that are associated with that definition, and should be included after the type being compiled. Currently, the linker ignores these statements, as there are none generated that are relevant in the linking operation. A challenge to supporting such associated statements is that the linker operates on "declare" expressions, and replaces those expressions with other expressions. It does not have the capability to append statements after the whole type. The linker actually faces this challenge with statements from the `ConstantPool` as well, and solves this problem by generating an IIFE expression that executes the statements and then returns the definition expression. Previously, an `EmitScope` processed the definition and converted it to an expression, as well as collected constant statements from a `ConstantPool`. A special `IifeEmitScope` implementation was used when emitting into a context where top-level constant statements couldn't be added at all, and uses the IIFE strategy in this case. This commit adds blanket support for associated statements to the linker using this IIFE strategy. The main `EmitScope` now uses the IIFE strategy to emit associated statements, and `IifeEmitScope` has been renamed to `LocalEmitScope`. Now, the `LocalEmitScope` represents constant statements as associated statements to the main `EmitScope` implementation, so they get included in the IIFE as well. Tests are adjusted/added to cover this new behavior. This is a refactoring commit because no live generated code is affected - there are no cases where associated statements are present in linked definitions today. PR Close #45024
… ids (#45024) Angular contains an NgModule registry, which allows a user to declare NgModules with string ids and retrieve them via those ids, using the `getNgModuleById` API. Previously, we attempted to structure this registration in a clever fashion to allow for tree-shaking of registered NgModules (that is, those with ids). This sort of worked due to the accidental alignment of behaviors from the different tree-shakers involved. However, this trick relies on the generation of `.ngfactory` files and how they're specifically processed in various bundling scenarios. We intend to remove `.ngfactory` files, hence we can no longer rely on them in this way. The correct solution here is to recognize that `@NgModule({id})` is inherently declaring a global side-effect, and such classes should not really be eligible for tree-shaking in the first place. This commit removes all the old registration machinery, and standardizes on generating a side- effectful call to `registerNgModuleType` for NgModules that have ids. There is some risk here that NgModules with unnecessary `id`s may not tree-shake as a result of this change, whereas they would have in previous circumstances. The fix here should be to remove the `id` if it's not needed. Specifying an `id` is a request that the NgModule be retained regardless of any other references, in case it is later looked up by string id. PR Close #45024
…5024) Previously, the `TraitCompiler` would naively consider a compilation as failed if either analysis or resolution produced any diagnostics. This commit adjusts the logic to only consider error diagnostics, which allows warnings to be produced from `DecoratorHandler`s. This is a precursor commit to introducing such a warning. As such, the logic here will be tested in the next commit. PR Close #45024
#45024) In early versions of Angular, it was sometimes necessary to provide a `moduleId` to `@Component` metadata, and the common pattern for doing this was to set `moduleId: module.id`. This relied on the bundler to fill in a value for `module.id`. However, due to the superficial similarity between `Component.moduleId` and `NgModule.id`, many users ended up setting `id: module.id` in their NgModules. This is an anti-pattern that has a few negative effects, including preventing the NgModule from tree-shaking properly. This commit changes the compiler to ignore `id: module.id` in NgModules, and instead provide a warning which suggests removing the line entirely. PR Close #45024
…r#45024) The `compileNgModule` operation previously supported a flag `emitInline`, which controlled whether template scoping information for the NgModule was emitted directly into the compiled NgModule definition, or whether an associated statement was generated which patched the information onto the NgModule definition. Both options are useful in different contexts. This commit changes this flag to an enum (and renames it), which allows for a third option - do not emit any template scoping information. This option is added to better represent the actual behavior of the Angular Linker, which sometimes configures `compileNgModule` to use the side-effectful statement generation but which does not actually emit such associated statements. In other words, the linker effectively does not generate scoping information for NgModules at all (in some configurations) and this option more directly expresses that behavior. This is a refactoring as no generated code is changed as a result of introducing this flag, due to the linker's behavior of not emitting associated statements. PR Close angular#45024
…ld (angular#45024) When `@angular/compiler` processes metadata and compiles a definition field, it might also choose to return statements that are associated with that definition, and should be included after the type being compiled. Currently, the linker ignores these statements, as there are none generated that are relevant in the linking operation. A challenge to supporting such associated statements is that the linker operates on "declare" expressions, and replaces those expressions with other expressions. It does not have the capability to append statements after the whole type. The linker actually faces this challenge with statements from the `ConstantPool` as well, and solves this problem by generating an IIFE expression that executes the statements and then returns the definition expression. Previously, an `EmitScope` processed the definition and converted it to an expression, as well as collected constant statements from a `ConstantPool`. A special `IifeEmitScope` implementation was used when emitting into a context where top-level constant statements couldn't be added at all, and uses the IIFE strategy in this case. This commit adds blanket support for associated statements to the linker using this IIFE strategy. The main `EmitScope` now uses the IIFE strategy to emit associated statements, and `IifeEmitScope` has been renamed to `LocalEmitScope`. Now, the `LocalEmitScope` represents constant statements as associated statements to the main `EmitScope` implementation, so they get included in the IIFE as well. Tests are adjusted/added to cover this new behavior. This is a refactoring commit because no live generated code is affected - there are no cases where associated statements are present in linked definitions today. PR Close angular#45024
… ids (angular#45024) Angular contains an NgModule registry, which allows a user to declare NgModules with string ids and retrieve them via those ids, using the `getNgModuleById` API. Previously, we attempted to structure this registration in a clever fashion to allow for tree-shaking of registered NgModules (that is, those with ids). This sort of worked due to the accidental alignment of behaviors from the different tree-shakers involved. However, this trick relies on the generation of `.ngfactory` files and how they're specifically processed in various bundling scenarios. We intend to remove `.ngfactory` files, hence we can no longer rely on them in this way. The correct solution here is to recognize that `@NgModule({id})` is inherently declaring a global side-effect, and such classes should not really be eligible for tree-shaking in the first place. This commit removes all the old registration machinery, and standardizes on generating a side- effectful call to `registerNgModuleType` for NgModules that have ids. There is some risk here that NgModules with unnecessary `id`s may not tree-shake as a result of this change, whereas they would have in previous circumstances. The fix here should be to remove the `id` if it's not needed. Specifying an `id` is a request that the NgModule be retained regardless of any other references, in case it is later looked up by string id. PR Close angular#45024
…gular#45024) Previously, the `TraitCompiler` would naively consider a compilation as failed if either analysis or resolution produced any diagnostics. This commit adjusts the logic to only consider error diagnostics, which allows warnings to be produced from `DecoratorHandler`s. This is a precursor commit to introducing such a warning. As such, the logic here will be tested in the next commit. PR Close angular#45024
angular#45024) In early versions of Angular, it was sometimes necessary to provide a `moduleId` to `@Component` metadata, and the common pattern for doing this was to set `moduleId: module.id`. This relied on the bundler to fill in a value for `module.id`. However, due to the superficial similarity between `Component.moduleId` and `NgModule.id`, many users ended up setting `id: module.id` in their NgModules. This is an anti-pattern that has a few negative effects, including preventing the NgModule from tree-shaking properly. This commit changes the compiler to ignore `id: module.id` in NgModules, and instead provide a warning which suggests removing the line entirely. PR Close angular#45024
…r#45024) The `compileNgModule` operation previously supported a flag `emitInline`, which controlled whether template scoping information for the NgModule was emitted directly into the compiled NgModule definition, or whether an associated statement was generated which patched the information onto the NgModule definition. Both options are useful in different contexts. This commit changes this flag to an enum (and renames it), which allows for a third option - do not emit any template scoping information. This option is added to better represent the actual behavior of the Angular Linker, which sometimes configures `compileNgModule` to use the side-effectful statement generation but which does not actually emit such associated statements. In other words, the linker effectively does not generate scoping information for NgModules at all (in some configurations) and this option more directly expresses that behavior. This is a refactoring as no generated code is changed as a result of introducing this flag, due to the linker's behavior of not emitting associated statements. PR Close angular#45024
…ld (angular#45024) When `@angular/compiler` processes metadata and compiles a definition field, it might also choose to return statements that are associated with that definition, and should be included after the type being compiled. Currently, the linker ignores these statements, as there are none generated that are relevant in the linking operation. A challenge to supporting such associated statements is that the linker operates on "declare" expressions, and replaces those expressions with other expressions. It does not have the capability to append statements after the whole type. The linker actually faces this challenge with statements from the `ConstantPool` as well, and solves this problem by generating an IIFE expression that executes the statements and then returns the definition expression. Previously, an `EmitScope` processed the definition and converted it to an expression, as well as collected constant statements from a `ConstantPool`. A special `IifeEmitScope` implementation was used when emitting into a context where top-level constant statements couldn't be added at all, and uses the IIFE strategy in this case. This commit adds blanket support for associated statements to the linker using this IIFE strategy. The main `EmitScope` now uses the IIFE strategy to emit associated statements, and `IifeEmitScope` has been renamed to `LocalEmitScope`. Now, the `LocalEmitScope` represents constant statements as associated statements to the main `EmitScope` implementation, so they get included in the IIFE as well. Tests are adjusted/added to cover this new behavior. This is a refactoring commit because no live generated code is affected - there are no cases where associated statements are present in linked definitions today. PR Close angular#45024
… ids (angular#45024) Angular contains an NgModule registry, which allows a user to declare NgModules with string ids and retrieve them via those ids, using the `getNgModuleById` API. Previously, we attempted to structure this registration in a clever fashion to allow for tree-shaking of registered NgModules (that is, those with ids). This sort of worked due to the accidental alignment of behaviors from the different tree-shakers involved. However, this trick relies on the generation of `.ngfactory` files and how they're specifically processed in various bundling scenarios. We intend to remove `.ngfactory` files, hence we can no longer rely on them in this way. The correct solution here is to recognize that `@NgModule({id})` is inherently declaring a global side-effect, and such classes should not really be eligible for tree-shaking in the first place. This commit removes all the old registration machinery, and standardizes on generating a side- effectful call to `registerNgModuleType` for NgModules that have ids. There is some risk here that NgModules with unnecessary `id`s may not tree-shake as a result of this change, whereas they would have in previous circumstances. The fix here should be to remove the `id` if it's not needed. Specifying an `id` is a request that the NgModule be retained regardless of any other references, in case it is later looked up by string id. PR Close angular#45024
…gular#45024) Previously, the `TraitCompiler` would naively consider a compilation as failed if either analysis or resolution produced any diagnostics. This commit adjusts the logic to only consider error diagnostics, which allows warnings to be produced from `DecoratorHandler`s. This is a precursor commit to introducing such a warning. As such, the logic here will be tested in the next commit. PR Close angular#45024
angular#45024) In early versions of Angular, it was sometimes necessary to provide a `moduleId` to `@Component` metadata, and the common pattern for doing this was to set `moduleId: module.id`. This relied on the bundler to fill in a value for `module.id`. However, due to the superficial similarity between `Component.moduleId` and `NgModule.id`, many users ended up setting `id: module.id` in their NgModules. This is an anti-pattern that has a few negative effects, including preventing the NgModule from tree-shaking properly. This commit changes the compiler to ignore `id: module.id` in NgModules, and instead provide a warning which suggests removing the line entirely. PR Close angular#45024
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. |
This PR refactors and updates Angular's NgModule registration scheme, which supports lazy loading in bundling environments where direct references are not available to dynamically loaded ES modules, and global side effects must be used to rendezvous with loaded code.
In particular, this PR addresses an issue where previously this system relied upon such side effects in
.ngfactory
shim files, which are deprecated and in the process of being removed.See individual commits for more details.