fix(core): ensure that autoRegisterModuleById registration in ɵɵdefineNgModule is not DCE-ed by closure #42529
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
IgorMinar
added
type: bug/fix
action: review
|
You can preview a04f0b0 at https://pr42529-a04f0b0.ngbuilds.io/. |
alxhub
approved these changes
Jun 9, 2021
…eNgModule is not DCE-ed by closure Previously the autoRegisterModuleById registration was marked with noSideEffects wrapper to ensure that we don't end up retaining all NgModules. However the return value was not referenced by anything, so closure compiler removed it because it determined that this code has no side effects and is not referenced by anyone. This issue affects apps that use Closure Compiler and also rely on https://angular.io/api/core/getModuleFactory to retrieve factories by ID. This combination is used heavily in google3, especially in Pantheon. Fixes b/188453434
IgorMinar
force-pushed
the
fix/defineNgModule-closure-bug
branch
from
a04f0b0
to
de6c55a
Compare
|
You can preview de6c55a at https://pr42529-de6c55a.ngbuilds.io/. |
IgorMinar
added
action: merge
alxhub
approved these changes
Jun 9, 2021
| }; | ||
| if (def.id != null) { | ||
| noSideEffects(() => { | ||
| return noSideEffects(() => { |
There was a problem hiding this comment.
Quick note (not a blocker for this PR, we can do that later): the PR description looks great and provide a useful insight into why this change is needed. I was thinking if we can partially reuse it as a comment in this function. Thank you.
alxhub
pushed a commit
that referenced
this pull request
Jun 9, 2021
…eNgModule is not DCE-ed by closure (#42529) Previously the autoRegisterModuleById registration was marked with noSideEffects wrapper to ensure that we don't end up retaining all NgModules. However the return value was not referenced by anything, so closure compiler removed it because it determined that this code has no side effects and is not referenced by anyone. This issue affects apps that use Closure Compiler and also rely on https://angular.io/api/core/getModuleFactory to retrieve factories by ID. This combination is used heavily in google3, especially in Pantheon. Fixes b/188453434 PR Close #42529
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously the autoRegisterModuleById registration was marked with noSideEffects wrapper to ensure that we don't end up retaining all NgModules.
However the return value was not referenced by anything, so closure compiler removed it because it determined that this code has no side effects and is not referenced by anyone.
This issue affects apps that use Closure Compiler and also rely on https://angular.io/api/core/getModuleFactory to retrieve factories by ID. This combination is used heavily in google3, especially in Pantheon.