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): auto register NgModules with ID #33663

Closed
wants to merge 3 commits into from

Conversation

@atscott
Copy link
Contributor

atscott commented Nov 7, 2019

For projects that use lazy loaded modules, we've observed that tree shaking in tests sometimes results in the ngfactory file for the lazy loaded module being removed. This results in the ng_module_factory_registration map not registering the module's id so when code later attempts to retrieve the factory, it fails. This is because ngfactory files have a side effect of creating an NgModuleFactory (which calls registerNgModuleType in the constructor). This PR keeps track of modules at the point of creation, providing a backup for retrieving it if the ngfactory was somehow tree-shaken away.

This commit also reverts the changes to TestBed that dealt with restoring the registered modules in ng_module_factory_registration.ts. These reverts are necessary because this change would make it so that even if a module is not registered in the map there, it is still retrieved from the map from defineNgModule. The original fixes in TestBed were not necessary: test setup in google3 has since been modified to not need the state restore because it now recompiles the lazy loaded modules with TestBed before each test, ensuring new provider overrides take effect.

We should run global presubmit with this PR to ensure it doesn't break anything.

@atscott atscott requested a review from alxhub Nov 7, 2019
@googlebot googlebot added the cla: yes label Nov 7, 2019
@atscott atscott mentioned this pull request Nov 7, 2019
0 of 14 tasks complete
@@ -376,6 +380,9 @@ export function ɵɵdefineNgModule<T>(def: {
schemas: def.schemas || null,
id: def.id || null,
};
if (def.id != null) {
autoRegisterModuleById[def.id] = def.type as unknown as NgModuleType;

This comment has been minimized.

Copy link
@alxhub

alxhub Nov 7, 2019

Contributor

I believe this will break tree-shaking, because Closure/Terser will treat this as a side effect and not be able to remove unused NgModules as a result.

@atscott atscott force-pushed the atscott:autoregister branch from 22a8eca to 017e885 Nov 7, 2019
@atscott atscott added the comp: core label Nov 7, 2019
@ngbot ngbot bot added this to the needsTriage milestone Nov 7, 2019
@atscott atscott force-pushed the atscott:autoregister branch 3 times, most recently from 9c6ce33 to bb70bf9 Nov 7, 2019
@atscott atscott force-pushed the atscott:autoregister branch from bb70bf9 to 7380be0 Nov 8, 2019
@atscott

This comment has been minimized.

Copy link
Contributor Author

atscott commented Nov 8, 2019

atscott added 2 commits Nov 8, 2019
…dules with TestBed (#32944)

This commit reverts 63256b5.
…each test (#32872)

This commit reverts 475e36a.
@atscott

This comment has been minimized.

Copy link
Contributor Author

atscott commented Nov 8, 2019

Compare these presubmits (after revert to TestBed) to the ones before the revert.
ivy presubmit
presubmit for project with custom lazy loading

Edit: confirmed no regressions in either run. Should be good to merge but still would like to check global presubmit as well.

@atscott atscott marked this pull request as ready for review Nov 8, 2019
@atscott atscott requested a review from angular/fw-core as a code owner Nov 8, 2019
@atscott atscott requested a review from AndrewKushnir Nov 8, 2019
@mhevery
mhevery approved these changes Nov 8, 2019
@atscott

This comment has been minimized.

Copy link
Contributor Author

atscott commented Nov 11, 2019

@kara kara requested a review from alxhub Nov 11, 2019
@alxhub
alxhub approved these changes Nov 12, 2019
@kara kara closed this in a972e4c Nov 12, 2019
kara added a commit that referenced this pull request Nov 12, 2019
kara added a commit that referenced this pull request Nov 12, 2019
…each test (#32872) (#33663)

This commit reverts 475e36a.

PR Close #33663
kara added a commit that referenced this pull request Nov 12, 2019
kara added a commit that referenced this pull request Nov 12, 2019
kara added a commit that referenced this pull request Nov 12, 2019
…each test (#32872) (#33663)

This commit reverts 475e36a.

PR Close #33663
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

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.
Projects
None yet
6 participants
You can’t perform that action at this time.