Two related fixes for NgModule compilation output#31939
Conversation
600b2a7 to
f75a8b0
Compare
petebacondarwin
left a comment
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Need to update the comment above this.
There was a problem hiding this comment.
In the jit_compiler_facade you replaced this with guardJitDefinition: false,. Why not here?
There was a problem hiding this comment.
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.
f75a8b0 to
82f1749
Compare
515f4cd to
6a87fe3
Compare
ee27f64 to
cffd347
Compare
cffd347 to
f3fb5cc
Compare
f3fb5cc to
943c283
Compare
IgorMinar
left a comment
There was a problem hiding this comment.
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
ngJitModevariable.
- 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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
The CLI already reads this if present: https://github.com/angular/angular-cli/blob/master/packages/angular_devkit/build_angular/src/angular-cli-files/models/webpack-configs/common.ts#L362-L372
There was a problem hiding this comment.
aha! I did not know that. In that case CLI is good to go. thanks for the info.
IgorMinar
left a comment
There was a problem hiding this comment.
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.
943c283 to
cb913c5
Compare
|
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. |
No description provided.