Skip to content

Commit

Permalink
feat(compiler-cli): no longer re-export external symbols by default
Browse files Browse the repository at this point in the history
With #28594 we refactored the `@angular/compiler` slightly to
allow opting out from external symbol re-exports which are
enabled by default.

Since symbol re-exports only benefit projects which have a
very strict dependency enforcement, external symbols should
not be re-exported by default as this could grow the size of
factory files and cause unexpected behavior with Angular's
AOT symbol resolving (e.g. see: #25644).

Additionally "ngtsc" also does not provide any way of using
external symbol re-exports, so this means that NGC partially
matches the behavior of "ngtsc" then (unless explicitly opted-out).

Internally at Google, symbol re-exports need to be still enabled
since Google has a very strict dependency enforcement (even of
produced JavaScript output), so the `ng_module` Bazel rule will
enable the symbol re-exports by default when running within Blaze.

Fixes #25644.
  • Loading branch information
devversion committed Feb 12, 2019
1 parent 1e64f37 commit e38bebd
Show file tree
Hide file tree
Showing 9 changed files with 585 additions and 291 deletions.
4 changes: 4 additions & 0 deletions packages/bazel/src/ng_module.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,10 @@ def _ngc_tsconfig(ctx, files, srcs, **kwargs):
"enableSummariesForJit": is_legacy_ngc,
"enableIvy": _enable_ivy_value(ctx),
"fullTemplateTypeCheck": ctx.attr.type_check,
# In Blaze we still want to use the symbol factory re-exports in order to
# not break existing apps inside google3. Disabling this in google3 would
# cause strict dependency failures for emitted files.
"createExternalSymbolFactoryReexports": (not _is_bazel()),
# FIXME: wrong place to de-dupe
"expectedOut": depset([o.path for o in expected_outs]).to_list(),
}
Expand Down
9 changes: 9 additions & 0 deletions packages/compiler-cli/src/transformers/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,15 @@ export interface CompilerOptions extends ts.CompilerOptions {

/** @internal */
collectAllErrors?: boolean;

/**
* Whether NGC should generate re-exports for external symbols which are referenced
* in Angular metadata (e.g. @Component, @Inject, @ViewChild). This can be enabled in
* order to avoid dynamically generated module dependencies which can break strict
* dependency enforcements. This is not enabled by default.
* Read more about this here: https://github.com/angular/angular/issues/25644.
*/
createExternalSymbolFactoryReexports?: boolean;
}

export interface CompilerHost extends ts.CompilerHost {
Expand Down
1 change: 1 addition & 0 deletions packages/compiler-cli/src/transformers/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -942,6 +942,7 @@ function getAotCompilerOptions(options: CompilerOptions): AotCompilerOptions {
fullTemplateTypeCheck: options.fullTemplateTypeCheck,
allowEmptyCodegenFiles: options.allowEmptyCodegenFiles,
enableIvy: options.enableIvy,
createExternalSymbolFactoryReexports: options.createExternalSymbolFactoryReexports,
};
}

Expand Down
223 changes: 187 additions & 36 deletions packages/compiler-cli/test/ngc_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1076,15 +1076,17 @@ describe('ngc transformer command-line', () => {
});
});

it('should be able to compile multiple libraries with summaries', () => {
// Note: we need to emit the generated code for the libraries
// into the node_modules, as that is the only way that we
// currently support when using summaries.
// TODO(tbosch): add support for `paths` to our CompilerHost.fileNameToModuleName
// and then use `paths` here instead of writing to node_modules.

// Angular
write('tsconfig-ng.json', `{
describe('with external symbol re-exports enabled', () => {

it('should be able to compile multiple libraries with summaries', () => {
// Note: we need to emit the generated code for the libraries
// into the node_modules, as that is the only way that we
// currently support when using summaries.
// TODO(tbosch): add support for `paths` to our CompilerHost.fileNameToModuleName
// and then use `paths` here instead of writing to node_modules.

// Angular
write('tsconfig-ng.json', `{
"extends": "./tsconfig-base.json",
"angularCompilerOptions": {
"generateCodeForLibraries": true,
Expand All @@ -1100,69 +1102,68 @@ describe('ngc transformer command-line', () => {
]
}`);

// Lib 1
write('lib1/tsconfig-lib1.json', `{
// Lib 1
write('lib1/tsconfig-lib1.json', `{
"extends": "../tsconfig-base.json",
"angularCompilerOptions": {
"generateCodeForLibraries": false,
"enableSummariesForJit": true
"enableSummariesForJit": true,
"createExternalSymbolFactoryReexports": true
},
"compilerOptions": {
"rootDir": ".",
"outDir": "../node_modules/lib1_built"
}
}`);
write('lib1/module.ts', `
write('lib1/module.ts', `
import {NgModule} from '@angular/core';
export function someFactory(): any { return null; }
@NgModule({
providers: [{provide: 'foo', useFactory: someFactory}]
})
export class Module {}
`);
write('lib1/class1.ts', `export class Class1 {}`);
write('lib1/class1.ts', `export class Class1 {}`);

// Lib 2
write('lib2/tsconfig-lib2.json', `{
// Lib 2
write('lib2/tsconfig-lib2.json', `{
"extends": "../tsconfig-base.json",
"angularCompilerOptions": {
"generateCodeForLibraries": false,
"enableSummariesForJit": true
"enableSummariesForJit": true,
"createExternalSymbolFactoryReexports": true
},
"compilerOptions": {
"rootDir": ".",
"outDir": "../node_modules/lib2_built"
}
}`);
write('lib2/module.ts', `
write('lib2/module.ts', `
export {Module} from 'lib1_built/module';
`);
write('lib2/class2.ts', `
import {Class1} from 'lib1_built/class1';
export class Class2 {
constructor(class1: Class1) {}
}
`);
write('lib2/class2.ts', `
import {Class1} from 'lib1_built/class1';
export class Class2 {
constructor(class1: Class1) {}
}
`);

// Application
write('app/tsconfig-app.json', `{
// Application
write('app/tsconfig-app.json', `{
"extends": "../tsconfig-base.json",
"angularCompilerOptions": {
"generateCodeForLibraries": false,
"enableSummariesForJit": true
"enableSummariesForJit": true,
"createExternalSymbolFactoryReexports": true
},
"compilerOptions": {
"rootDir": ".",
"outDir": "../built/app"
}
}`);
write('app/main.ts', `
write('app/main.ts', `
import {NgModule, Inject} from '@angular/core';
import {Module} from 'lib2_built/module';
@NgModule({
imports: [Module]
})
Expand All @@ -1171,6 +1172,149 @@ describe('ngc transformer command-line', () => {
}
`);

expect(main(['-p', path.join(basePath, 'lib1', 'tsconfig-lib1.json')], errorSpy)).toBe(0);
expect(main(['-p', path.join(basePath, 'lib2', 'tsconfig-lib2.json')], errorSpy)).toBe(0);
expect(main(['-p', path.join(basePath, 'app', 'tsconfig-app.json')], errorSpy)).toBe(0);

// library 1
// make `shouldExist` / `shouldNotExist` relative to `node_modules`
outDir = path.resolve(basePath, 'node_modules');
shouldExist('lib1_built/module.js');
shouldExist('lib1_built/module.ngsummary.json');
shouldExist('lib1_built/module.ngsummary.js');
shouldExist('lib1_built/module.ngsummary.d.ts');
shouldExist('lib1_built/module.ngfactory.js');
shouldExist('lib1_built/module.ngfactory.d.ts');

// library 2
// make `shouldExist` / `shouldNotExist` relative to `node_modules`
outDir = path.resolve(basePath, 'node_modules');
shouldExist('lib2_built/module.js');
shouldExist('lib2_built/module.ngsummary.json');
shouldExist('lib2_built/module.ngsummary.js');
shouldExist('lib2_built/module.ngsummary.d.ts');
shouldExist('lib2_built/module.ngfactory.js');
shouldExist('lib2_built/module.ngfactory.d.ts');

shouldExist('lib2_built/class2.ngsummary.json');
shouldNotExist('lib2_built/class2.ngsummary.js');
shouldNotExist('lib2_built/class2.ngsummary.d.ts');
shouldExist('lib2_built/class2.ngfactory.js');
shouldExist('lib2_built/class2.ngfactory.d.ts');

// app
// make `shouldExist` / `shouldNotExist` relative to `built`
outDir = path.resolve(basePath, 'built');
shouldExist('app/main.js');
});

it('should create external symbol re-exports', () => {
writeConfig(`{
"extends": "./tsconfig-base.json",
"angularCompilerOptions": {
"generateCodeForLibraries": false,
"createExternalSymbolFactoryReexports": true
}
}`);

write('test.ts', `
import {Injectable, NgZone} from '@angular/core';
@Injectable({providedIn: 'root'})
export class MyService {
constructor(public ngZone: NgZone) {}
}
`);

expect(main(['-p', basePath], errorSpy)).toBe(0);

shouldExist('test.js');
shouldExist('test.metadata.json');
shouldExist('test.ngsummary.json');
shouldExist('test.ngfactory.js');
shouldExist('test.ngfactory.d.ts');

const summaryJson = require(path.join(outDir, 'test.ngsummary.json'));
const factoryOutput = fs.readFileSync(path.join(outDir, 'test.ngfactory.js'), 'utf8');

expect(summaryJson['symbols'][0].name).toBe('MyService');
expect(summaryJson['symbols'][1])
.toEqual(jasmine.objectContaining({name: 'NgZone', importAs: 'NgZone_1'}));

expect(factoryOutput).toContain(`export { NgZone as NgZone_1 } from "@angular/core";`);
});
});

it('should be able to compile multiple libraries with summaries', () => {
// Lib 1
write('lib1/tsconfig-lib1.json', `{
"extends": "../tsconfig-base.json",
"angularCompilerOptions": {
"generateCodeForLibraries": false,
"enableSummariesForJit": true
},
"compilerOptions": {
"rootDir": ".",
"outDir": "../node_modules/lib1_built"
}
}`);
write('lib1/module.ts', `
import {NgModule} from '@angular/core';
export function someFactory(): any { return null; }
@NgModule({
providers: [{provide: 'foo', useFactory: someFactory}]
})
export class Module {}
`);
write('lib1/class1.ts', `export class Class1 {}`);

// Lib 2
write('lib2/tsconfig-lib2.json', `{
"extends": "../tsconfig-base.json",
"angularCompilerOptions": {
"generateCodeForLibraries": false,
"enableSummariesForJit": true
},
"compilerOptions": {
"rootDir": ".",
"outDir": "../node_modules/lib2_built"
}
}`);
write('lib2/module.ts', `export {Module} from 'lib1_built/module';`);
write('lib2/class2.ts', `
import {Class1} from 'lib1_built/class1';
export class Class2 {
constructor(class1: Class1) {}
}
`);

// Application
write('app/tsconfig-app.json', `{
"extends": "../tsconfig-base.json",
"angularCompilerOptions": {
"generateCodeForLibraries": false,
"enableSummariesForJit": true
},
"compilerOptions": {
"rootDir": ".",
"outDir": "../built/app"
}
}`);
write('app/main.ts', `
import {NgModule, Inject} from '@angular/core';
import {Module} from 'lib2_built/module';
@NgModule({
imports: [Module]
})
export class AppModule {
constructor(@Inject('foo') public foo: any) {}
}
`);

expect(main(['-p', path.join(basePath, 'lib1', 'tsconfig-lib1.json')], errorSpy)).toBe(0);
expect(main(['-p', path.join(basePath, 'lib2', 'tsconfig-lib2.json')], errorSpy)).toBe(0);
expect(main(['-p', path.join(basePath, 'app', 'tsconfig-app.json')], errorSpy)).toBe(0);
Expand All @@ -1189,17 +1333,24 @@ describe('ngc transformer command-line', () => {
// make `shouldExist` / `shouldNotExist` relative to `node_modules`
outDir = path.resolve(basePath, 'node_modules');
shouldExist('lib2_built/module.js');

// "module.ts" re-exports an external symbol and will therefore
// have a summary JSON file and its corresponding JIT summary.
shouldExist('lib2_built/module.ngsummary.json');
shouldExist('lib2_built/module.ngsummary.js');
shouldExist('lib2_built/module.ngsummary.d.ts');
shouldExist('lib2_built/module.ngfactory.js');
shouldExist('lib2_built/module.ngfactory.d.ts');
// "module.ts" only re-exports an external symbol and the AOT compiler does not
// need to generate anything. Therefore there should be no factory files.
shouldNotExist('lib2_built/module.ngfactory.js');
shouldNotExist('lib2_built/module.ngfactory.d.ts');

shouldExist('lib2_built/class2.ngsummary.json');
shouldNotExist('lib2_built/class2.ngsummary.js');
shouldNotExist('lib2_built/class2.ngsummary.d.ts');
shouldExist('lib2_built/class2.ngfactory.js');
shouldExist('lib2_built/class2.ngfactory.d.ts');
// We don't expect factories here because the "class2.ts" file
// just exports a class that does not produce any AOT code.
shouldNotExist('lib2_built/class2.ngfactory.js');
shouldNotExist('lib2_built/class2.ngfactory.d.ts');

// app
// make `shouldExist` / `shouldNotExist` relative to `built`
Expand Down
2 changes: 1 addition & 1 deletion packages/compiler/src/aot/summary_serializer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export function serializeSummaries(
CompileTypeMetadata
}[],
createExternalSymbolReexports =
true): {json: string, exportAs: {symbol: StaticSymbol, exportAs: string}[]} {
false): {json: string, exportAs: {symbol: StaticSymbol, exportAs: string}[]} {
const toJsonSerializer = new ToJsonSerializer(symbolResolver, summaryResolver, srcFileName);

// for symbols, we use everything except for the class metadata itself
Expand Down
Loading

0 comments on commit e38bebd

Please sign in to comment.