The Wayback Machine - https://web.archive.org/web/20220611165756/https://github.com/angular/angular/pull/45024
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

Update to NgModule Registration #45024

Closed
wants to merge 5 commits into from

Conversation

alxhub
Copy link
Contributor

@alxhub alxhub commented Feb 9, 2022

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.

@alxhub alxhub force-pushed the ngtsc/register-ngmodule-id branch from 4784321 to 4a4f86c Compare Feb 9, 2022
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Looks great, thanks @alxhub 👍

@pullapprove pullapprove bot requested a review from AndrewKushnir Feb 9, 2022
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Reviewed-for: public-api

@pullapprove pullapprove bot requested a review from jessicajaniuk Feb 9, 2022
aio/content/errors/NG10003.md Outdated Show resolved Hide resolved
@pullapprove pullapprove bot requested review from atscott Feb 9, 2022
@alxhub alxhub force-pushed the ngtsc/register-ngmodule-id branch 2 times, most recently from e0225c6 to 30a5b7a Compare Feb 9, 2022
@alxhub alxhub force-pushed the ngtsc/register-ngmodule-id branch from 30a5b7a to a83e728 Compare Feb 9, 2022
JoostK
JoostK approved these changes Feb 9, 2022
atscott
atscott approved these changes Feb 9, 2022
Copy link
Contributor

@atscott atscott left a comment

reviewed-for: public-api

@alxhub alxhub force-pushed the ngtsc/register-ngmodule-id branch from a83e728 to e1c2282 Compare Feb 10, 2022
@alxhub alxhub added state: blocked target: major labels Feb 10, 2022
@alxhub
Copy link
Contributor Author

@alxhub alxhub commented Feb 10, 2022

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);
Copy link
Member

@JoostK JoostK Feb 10, 2022

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

@alxhub alxhub force-pushed the ngtsc/register-ngmodule-id branch from e1c2282 to f3509ff Compare Feb 11, 2022
@alxhub alxhub force-pushed the ngtsc/register-ngmodule-id branch 4 times, most recently from 759ada1 to b4c34d9 Compare Feb 12, 2022
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Mar 21, 2022

Note to the Caretaker: this PR will require an additional change in g3, please include this change into the sync.

alxhub added 2 commits Mar 21, 2022
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.
@alxhub alxhub force-pushed the ngtsc/register-ngmodule-id branch from 56f13cd to d44c839 Compare Mar 21, 2022
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Mar 22, 2022

@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Mar 22, 2022

@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.

@AndrewKushnir AndrewKushnir added action: merge and removed action: presubmit labels Mar 22, 2022
alxhub added 3 commits Mar 22, 2022
… 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.
@alxhub alxhub force-pushed the ngtsc/register-ngmodule-id branch from 4978d0d to 1b90775 Compare Mar 22, 2022
@alxhub
Copy link
Contributor Author

@alxhub alxhub commented Mar 22, 2022

This PR was merged into the repository by commit 8155428.

@alxhub alxhub closed this in 72e7d09 Mar 22, 2022
alxhub added a commit that referenced this issue Mar 22, 2022
…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
alxhub added a commit that referenced this issue Mar 22, 2022
… 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
alxhub added a commit that referenced this issue Mar 22, 2022
…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
alxhub added a commit that referenced this issue Mar 22, 2022
#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
PiyushAgrawal1243 pushed a commit to PiyushAgrawal1243/angular that referenced this issue Mar 30, 2022
…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
PiyushAgrawal1243 pushed a commit to PiyushAgrawal1243/angular that referenced this issue Mar 30, 2022
…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
PiyushAgrawal1243 pushed a commit to PiyushAgrawal1243/angular that referenced this issue Mar 30, 2022
… 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
PiyushAgrawal1243 pushed a commit to PiyushAgrawal1243/angular that referenced this issue Mar 30, 2022
…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
PiyushAgrawal1243 pushed a commit to PiyushAgrawal1243/angular that referenced this issue Mar 30, 2022
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
josmar-crwdstffng pushed a commit to josmar-crwdstffng/angular that referenced this issue Apr 8, 2022
…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
josmar-crwdstffng pushed a commit to josmar-crwdstffng/angular that referenced this issue Apr 8, 2022
…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
josmar-crwdstffng pushed a commit to josmar-crwdstffng/angular that referenced this issue Apr 8, 2022
… 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
josmar-crwdstffng pushed a commit to josmar-crwdstffng/angular that referenced this issue Apr 8, 2022
…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
josmar-crwdstffng pushed a commit to josmar-crwdstffng/angular that referenced this issue Apr 8, 2022
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
@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Apr 22, 2022

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 Apr 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge comp: core target: major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants