Skip to content

Two related fixes for NgModule compilation output#31939

Closed
alxhub wants to merge 2 commits into
angular:masterfrom
alxhub:ngtsc/module/shaking
Closed

Two related fixes for NgModule compilation output#31939
alxhub wants to merge 2 commits into
angular:masterfrom
alxhub:ngtsc/module/shaking

Conversation

@alxhub
Copy link
Copy Markdown
Member

@alxhub alxhub commented Jul 31, 2019

No description provided.

@alxhub alxhub requested review from a team July 31, 2019 22:42
@alxhub alxhub force-pushed the ngtsc/module/shaking branch 3 times, most recently from 600b2a7 to f75a8b0 Compare August 1, 2019 16:35
@alxhub alxhub requested a review from a team August 1, 2019 16:35
Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of nits. Also I didn't understand this sentence in the commit message:

2) it breaks tree-shaking to include, as it describes the full module scope
   graph (every declaration of every module in the app)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo THis

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to update the comment above this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed, thanks.

Comment thread packages/core/src/render3/jit/module.ts Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the jit_compiler_facade you replaced this with guardJitDefinition: false,. Why not here?

Copy link
Copy Markdown
Member Author

@alxhub alxhub Aug 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a JIT-specific interface, so there's no need to expose the guardJitDefinition switch. It's always going to be false. When the compiler facade calls the real compileNgModule it needs to provide that option as the module compiler runs in both JIT and AOT.

@alxhub alxhub added the area: compiler Issues related to `ngc`, Angular's template compiler label Aug 8, 2019
@ngbot ngbot Bot added this to the needsTriage milestone Aug 8, 2019
@alxhub alxhub force-pushed the ngtsc/module/shaking branch from f75a8b0 to 82f1749 Compare August 14, 2019 18:21
@alxhub alxhub force-pushed the ngtsc/module/shaking branch 2 times, most recently from 515f4cd to 6a87fe3 Compare August 21, 2019 23:21
@alxhub alxhub force-pushed the ngtsc/module/shaking branch 2 times, most recently from ee27f64 to cffd347 Compare September 23, 2019 09:34
@alxhub alxhub force-pushed the ngtsc/module/shaking branch from cffd347 to f3fb5cc Compare October 9, 2019 16:34
@alxhub alxhub force-pushed the ngtsc/module/shaking branch from f3fb5cc to 943c283 Compare October 18, 2019 18:48
@alxhub alxhub added the target: major This PR is targeted for the next major release label Oct 18, 2019
Copy link
Copy Markdown
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this PR is a bit confusing because it has two unrelated parts.

  • the IIFE is valuable as it removes the burden from build-optimizer to do this kind of rewrite.
    • this change is all good, and will require a non-critical cleanup change in build-optimizer to remove the pass that adds this IIFE wrapper during build optimizations. it's great that we can do this during codegen and not have to correct the issue during each prod build. Can you please create a backlog item for the framework team? // heads up @filipesilva
  • the second part is all about supporting jit mode with prod optimizations, or rather ensuring that prod optimizations don't break the jit mode.
    • this change will require a modification in CLI to support the new terser flag array, as well as closure compiler config change in google3 to define ngJitMode variable.

Comment thread packages/compiler-cli/src/tooling.ts Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs missing

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the compiled code is not yet published to npm, and I don't expect us to publish the code to npm in the current shape/apis. please update, otherwise this will be confusing for future readers.

Comment thread packages/compiler-cli/src/tooling.ts Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about you flip this around. keep the GLOBAL_DEFS_FOR_TERSER aot specific and set ngJitMode to false there, and introduce GLOBAL_DEFS_FOR_TERSER_JIT which will have the flag set to true

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aha! I did not know that. In that case CLI is good to go. thanks for the info.

Copy link
Copy Markdown
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw I don't think that the ngModuleDef change is a feature, I think that this is a fix, because without it the JIT with prod optimizations is broken

ngModuleDef is a definition field for @NgModule, which passes module
metadata through to the runtime. This metadata is broadly divided into two
categories: metadata required for the runtime in all cases, and metadata
only required to support JIT compilation usage of the module.

There are two things about this JIT metadata that make it special:

1) it's not needed in AOT-compiled apps (the vast majority of apps)
2) it breaks tree-shaking to include, as it creates hard references from
the root NgModule to every declared/imported module/component/directive/pipe
in the app, regardless of whether they're actually used in the template.

To support JIT apps, this metadata must be included in generated code,
especially if that code is shipped to NPM. Therefore, to avoid breaking tree
shaking it must be _removed_ in AOT-only apps.

Previously, the metadata was patched onto the ngModuleDef via a special
setNgModuleScope call following the class, which was marked as "pure". Thus,
an optimizer like Terser would remove the call when optimizing a production
bundle.

This has a significant downside: it breaks production optimization of JIT
applications, which do need this metadata call to survive for runtime. JIT
optimization should not be coupled to the rest of tree shaking.

Instead, this commit introduces a compile-time constant `ngJitMode`. A
second argument to defineNgModule carries the JIT part of the definition,
which is guarded with `ngJitMode`:

```typescript
class FooModule {}
FooModule.ngModuleDef = defineNgModule(
  // The main module definition, included in all builds:
  {type: FooModule, id: 'foo'},
  // The JIT module definition, guarded by a compile-time constant:
  ngJitMode && {declarations: [FooCmp]},
);
```

Terser can be configured to define ngJitMode as false, which will result in
the JIT part of the definition shaking away. A suitable configuration is
provided in tooling.ts for use in the CLI.
This commit transforms the setClassMetadata calls generated by ngtsc from:

```typescript
/*@__PURE__*/ setClassMetadata(...);
```

to:

```typescript
/*@__PURE__*/ (function() {
  setClassMetadata(...);
})();
```

This is required for the build optimizer and side-effect-free analysis to
function correctly.
Copy link
Copy Markdown
Contributor

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we except users that can’t use AOT to provide set ngJitMode globally?

Example;
System JS users
Codepen/plunkr etc...

Edit: Never mind misread the statement 😥

@alxhub alxhub closed this Nov 18, 2019
@angular-automatic-lock-bot
Copy link
Copy Markdown

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 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area: compiler Issues related to `ngc`, Angular's template compiler cla: yes target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants