Skip to content

Commit 7f8c222

Browse files
dgp1130alxhub
authored andcommitted
fix(compiler): mark NgModuleFactory construction as not side effectful (angular#38147)
This allows Closure compiler to tree shake unused constructor calls to `NgModuleFactory`, which is otherwise considered side-effectful. The Angular compiler generates factory objects which are exported but typically not used, as they are only needed for compatibility with View Engine. This results in top-level constructor calls, such as: ```typescript export const FooNgFactory = new NgModuleFactory(Foo); ``` `NgModuleFactory` has a side-effecting constructor, so this statement cannot be tree shaken, even if `FooNgFactory` is never imported. The `NgModuleFactory` continues to reference its associated `NgModule` and prevents the module and all its unused dependencies from being tree shaken. This effectively prevents all components from being tree shaken, making Closure builds significantly larger than they should be. The fix here is to wrap `NgModuleFactory` constructor with `noSideEffects(() => /* ... */)`, which tricks the Closure compiler into assuming that the invoked function has no side effects. This allows it to tree-shake unused `NgModuleFactory()` constructors when they aren't imported. Since the factory can be removed, the module can also be removed (if nothing else references it), thus tree shaking unused components as expected. PR Close angular#38147
1 parent 887c350 commit 7f8c222

File tree

6 files changed

+18
-7
lines changed

6 files changed

+18
-7
lines changed

packages/compiler-cli/src/ngtsc/imports/src/core.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ const CORE_SUPPORTED_SYMBOLS = new Map<string, string>([
6565
['ɵɵInjectorDef', 'ɵɵInjectorDef'],
6666
['ɵɵNgModuleDefWithMeta', 'ɵɵNgModuleDefWithMeta'],
6767
['ɵNgModuleFactory', 'NgModuleFactory'],
68+
['ɵnoSideEffects', 'ɵnoSideEffects'],
6869
]);
6970

7071
const CORE_MODULE = '@angular/core';

packages/compiler-cli/src/ngtsc/shims/src/factory_generator.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,8 @@ export class FactoryGenerator implements PerFileShimGenerator, FactoryTracker {
6363
// because it won't miss any that do.
6464
const varLines = symbolNames.map(
6565
name => `export const ${
66-
name}NgFactory: i0.ɵNgModuleFactory<any> = new i0.ɵNgModuleFactory(${name});`);
66+
name}NgFactory: i0.ɵNgModuleFactory<any> = i0.ɵnoSideEffects(() => new i0.ɵNgModuleFactory(${
67+
name}));`);
6768
sourceText += [
6869
// This might be incorrect if the current package being compiled is Angular core, but it's
6970
// okay to leave in at type checking time. TypeScript can handle this reference via its path

packages/compiler-cli/test/ngtsc/fake_core/index.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,3 +103,5 @@ export interface QueryList<T>/* implements Iterable<T> */ {
103103
export type NgIterable<T> = Array<T>|Iterable<T>;
104104

105105
export class NgZone {}
106+
107+
export declare function ɵnoSideEffects<T>(fn: () => T): T;

packages/compiler-cli/test/ngtsc/ngtsc_spec.ts

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3538,7 +3538,9 @@ runInEachFileSystem(os => {
35383538
expect(factoryContents).toContain(`import * as i0 from '@angular/core';`);
35393539
expect(factoryContents).toContain(`import { NotAModule, TestModule } from './test';`);
35403540
expect(factoryContents)
3541-
.toContain(`export var TestModuleNgFactory = new i0.\u0275NgModuleFactory(TestModule);`);
3541+
.toContain(
3542+
'export var TestModuleNgFactory = ' +
3543+
'i0.ɵnoSideEffects(function () { return new i0.\u0275NgModuleFactory(TestModule); });');
35423544
expect(factoryContents).not.toContain(`NotAModuleNgFactory`);
35433545
expect(factoryContents).not.toContain('\u0275NonEmptyModule');
35443546

@@ -3677,11 +3679,14 @@ runInEachFileSystem(os => {
36773679
env.driveMain();
36783680

36793681
const factoryContents = env.getContents('test.ngfactory.js');
3680-
expect(normalize(factoryContents)).toBe(normalize(`
3681-
import * as i0 from "./r3_symbols";
3682-
import { TestModule } from './test';
3683-
export var TestModuleNgFactory = new i0.NgModuleFactory(TestModule);
3684-
`));
3682+
expect(normalize(factoryContents))
3683+
.toBe(
3684+
'import * as i0 from "./r3_symbols"; ' +
3685+
'import { TestModule } from \'./test\'; ' +
3686+
'export var TestModuleNgFactory = ' +
3687+
'i0.\u0275noSideEffects(function () { ' +
3688+
'return new i0.NgModuleFactory(TestModule); ' +
3689+
'});');
36853690
});
36863691

36873692
describe('file-level comments', () => {

packages/core/src/core_render3_private_export.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -292,5 +292,6 @@ export {
292292
ɵɵsanitizeUrl,
293293
ɵɵsanitizeUrlOrResourceUrl,
294294
} from './sanitization/sanitization';
295+
export {noSideEffects as ɵnoSideEffects} from './util/closure';
295296

296297
// clang-format on

packages/core/src/r3_symbols.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ export {ɵɵdefineNgModule} from './render3/definition';
2828
export {ɵɵFactoryDef} from './render3/interfaces/definition';
2929
export {setClassMetadata} from './render3/metadata';
3030
export {NgModuleFactory} from './render3/ng_module_ref';
31+
export {noSideEffects as ɵnoSideEffects} from './util/closure';
3132

3233

3334

0 commit comments

Comments
 (0)