feat(ivy): emit module scope metadata using pure function call by JoostK · Pull Request #29598 · angular/angular

JoostK · 2019-03-29T19:35:54Z

Prior to this change, all module metadata would be included in the
defineNgModule call that is set as the ngModuleDef field of module
types. Part of the metadata is scope information like declarations,
imports and exports that is used for computing the transitive module
scope in JIT environments, preventing those references from being
tree-shaken for production builds.

This change moves the metadata for scope computations to a pure function
call that patches the scope references onto the module type. Because the
function is marked pure, it may be tree-shaken out during production builds
such that references to declarations and exports are dropped, which in turn
allows for tree-shaken any declaration that is not otherwise referenced.

Fixes #28077, FW-1035

feat(ivy): exclude declarations from injector imports
Prior to this change, a module's imports and exports would be used verbatim
as an injectors' imports. This is detrimental for tree-shaking, as a
module's exports could reference declarations that would then prevent such
declarations from being eligible for tree-shaking.

Since an injector actually only needs NgModule references as its imports,
we may safely filter out any declarations from the list of module exports.
This makes them eligible for tree-shaking once again.

feat(ivy): do not emit empty providers/imports for defineInjector
The defineInjector function specifies its providers and imports array to
be optional, so if no providers/imports are present these keys may be
omitted. This commit updates the compiler to only generate the keys when
necessary.

Prior to this change, all module metadata would be included in the `defineNgModule` call that is set as the `ngModuleDef` field of module types. Part of the metadata is scope information like declarations, imports and exports that is used for computing the transitive module scope in JIT environments, preventing those references from being tree-shaken for production builds. This change moves the metadata for scope computations to a pure function call that patches the scope references onto the module type. Because the function is marked pure, it may be tree-shaken out during production builds such that references to declarations and exports are dropped, which in turn allows for tree-shaken any declaration that is not otherwise referenced. Fixes angular#28077, FW-1035

Prior to this change, a module's imports and exports would be used verbatim as an injectors' imports. This is detrimental for tree-shaking, as a module's exports could reference declarations that would then prevent such declarations from being eligible for tree-shaking. Since an injector actually only needs NgModule references as its imports, we may safely filter out any declarations from the list of module exports. This makes them eligible for tree-shaking once again.

The defineInjector function specifies its providers and imports array to be optional, so if no providers/imports are present these keys may be omitted. This commit updates the compiler to only generate the keys when necessary.

JoostK · 2019-03-30T15:00:25Z

+    if (scope !== null) {
+      const context = getSourceFile(node);
+      for (const exportRef of analysis.exports) {
+        if (isNgModule(exportRef.node, scope.compilation)) {


I am not too certain about using the module scope to figure out if the ref is a module. It does however conveniently provide us with information on both refs from the program, as well as resolved refs from typings files.

I actually like the approach of filtering out any exports from the scope, instead of filtering and including only known modules. It feels more robust.

alxhub · 2019-04-02T19:42:15Z

+        .toContain(
+            'i0.defineInjector({ factory: function TestModule_Factory(t) ' +
+            '{ return new (t || TestModule)(); }, imports: [[OtherModule, RouterModule.forRoot()],' +
+            '\n            OtherModule,\n            RouterModule] });');


Using the ts.Printer, you're going to get whitespace attached to existing nodes.

alxhub · 2019-04-02T19:28:52Z

+
+    const jsContents = env.getContents('test.js');
+    expect(jsContents)
+        .toContain(


We don't need to match the whole structure of defineInjector, it's enough to assert that it contains imports: [...]. You can also use a regex to ignore the whitespace.

alxhub · 2019-04-02T19:40:17Z

+    if (scope !== null) {
+      const context = getSourceFile(node);
+      for (const exportRef of analysis.exports) {
+        if (isNgModule(exportRef.node, scope.compilation)) {


I actually like the approach of filtering out any exports from the scope, instead of filtering and including only known modules. It feels more robust.

alxhub · 2019-04-02T19:42:15Z

+        .toContain(
+            'i0.defineInjector({ factory: function TestModule_Factory(t) ' +
+            '{ return new (t || TestModule)(); }, imports: [[OtherModule, RouterModule.forRoot()],' +
+            '\n            OtherModule,\n            RouterModule] });');


Using the ts.Printer, you're going to get whitespace attached to existing nodes.

alexeagle

payload-limits change looks fantastic to me (we probably shouldn't require separate reviewers for this file, fyi @ocombe )

kara · 2019-04-02T20:43:02Z

presubmit

alxhub · 2019-04-02T20:45:31Z

Presubmit

Prior to this change, a module's imports and exports would be used verbatim as an injectors' imports. This is detrimental for tree-shaking, as a module's exports could reference declarations that would then prevent such declarations from being eligible for tree-shaking. Since an injector actually only needs NgModule references as its imports, we may safely filter out any declarations from the list of module exports. This makes them eligible for tree-shaking once again. PR Close #29598

…9598) The defineInjector function specifies its providers and imports array to be optional, so if no providers/imports are present these keys may be omitted. This commit updates the compiler to only generate the keys when necessary. PR Close #29598

…ar#29598) Prior to this change, all module metadata would be included in the `defineNgModule` call that is set as the `ngModuleDef` field of module types. Part of the metadata is scope information like declarations, imports and exports that is used for computing the transitive module scope in JIT environments, preventing those references from being tree-shaken for production builds. This change moves the metadata for scope computations to a pure function call that patches the scope references onto the module type. Because the function is marked pure, it may be tree-shaken out during production builds such that references to declarations and exports are dropped, which in turn allows for tree-shaken any declaration that is not otherwise referenced. Fixes angular#28077, FW-1035 PR Close angular#29598

Prior to this change, a module's imports and exports would be used verbatim as an injectors' imports. This is detrimental for tree-shaking, as a module's exports could reference declarations that would then prevent such declarations from being eligible for tree-shaking. Since an injector actually only needs NgModule references as its imports, we may safely filter out any declarations from the list of module exports. This makes them eligible for tree-shaking once again. PR Close angular#29598

…gular#29598) The defineInjector function specifies its providers and imports array to be optional, so if no providers/imports are present these keys may be omitted. This commit updates the compiler to only generate the keys when necessary. PR Close angular#29598

angular-automatic-lock-bot · 2019-09-14T22:22:23Z

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

googlebot added the cla: yes label Mar 29, 2019

JoostK added the comp: ivy label Mar 29, 2019

ngbot Bot added this to the needsTriage milestone Mar 29, 2019

JoostK self-assigned this Mar 29, 2019

JoostK force-pushed the ngtsc-extern-module-def branch 2 times, most recently from 8ebef16 to 4041f11 Compare March 29, 2019 21:10

JoostK added 4 commits March 30, 2019 15:53

fixup! feat(ivy): exclude declarations from injector imports

993809b

JoostK force-pushed the ngtsc-extern-module-def branch from f7f5a9a to 9c94276 Compare March 30, 2019 14:54

JoostK commented Mar 30, 2019

View reviewed changes

JoostK marked this pull request as ready for review March 30, 2019 17:32

JoostK requested review from a team March 30, 2019 17:32

JoostK added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release labels Mar 31, 2019

JoostK removed their assignment Mar 31, 2019

angular deleted a comment from JoostK Apr 2, 2019

alxhub approved these changes Apr 2, 2019

View reviewed changes

alexeagle approved these changes Apr 2, 2019

View reviewed changes

kara removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Apr 2, 2019

kara added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit labels Apr 2, 2019

jasonaden closed this in 45c6360 Apr 2, 2019

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 14, 2019

JoostK deleted the ngtsc-extern-module-def branch September 15, 2019 13:05

Navigation Menu

Repository navigation

feat(ivy): emit module scope metadata using pure function call - #29598

feat(ivy): emit module scope metadata using pure function call#29598
JoostK wants to merge 4 commits into
angular:masterfrom
JoostK:ngtsc-extern-module-def

JoostK commented Mar 29, 2019 •

edited

Loading

Uh oh!

JoostK Mar 30, 2019

Uh oh!

alxhub Apr 2, 2019

Uh oh!

alxhub Apr 2, 2019

Uh oh!

alxhub Apr 2, 2019

Uh oh!

alxhub Apr 2, 2019

Uh oh!

alxhub Apr 2, 2019

Uh oh!

alexeagle left a comment

Uh oh!

kara commented Apr 2, 2019

Uh oh!

alxhub commented Apr 2, 2019

Uh oh!

angular-automatic-lock-bot Bot commented Sep 14, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

JoostK commented Mar 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoostK Mar 30, 2019

Choose a reason for hiding this comment

Uh oh!

alxhub Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

alxhub Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

alxhub Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

alxhub Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

alxhub Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

alexeagle left a comment

Choose a reason for hiding this comment

Uh oh!

kara commented Apr 2, 2019

Uh oh!

alxhub commented Apr 2, 2019

Uh oh!

angular-automatic-lock-bot Bot commented Sep 14, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JoostK commented Mar 29, 2019 •

edited

Loading