Skip to content
Permalink
Browse files

feat(compiler-cli): no longer re-export external symbols by default (#…

…28633)

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

Note that the common strict dependency enforcement for source
files does still work with external symbol re-exports disabled,
but there are also strict dependency checks that enforce strict
module dependencies also for _generated files_ (such as the
ngfactory files). This is how Google3 manages it's dependencies
and therefore external symbol re-exports need to be enabled within
Google3.

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

As mentioned before, internally at Google symbol re-exports need to
be still enabled, so the `ng_module` Bazel rule will enable the symbol
re-exports by default when running within Blaze.

Fixes #25644.

PR Close #28633
  • Loading branch information...
devversion authored and mhevery committed Feb 12, 2019
1 parent fc8f4f8 commit 91b7152852e86311a9c880e558ae0caefc7e5e26
@@ -245,6 +245,13 @@ def _ngc_tsconfig(ctx, files, srcs, **kwargs):
"enableSummariesForJit": is_legacy_ngc,
"enableIvy": _enable_ivy_value(ctx),
"fullTemplateTypeCheck": ctx.attr.type_check,
# In Google3 we still want to use the symbol factory re-exports in order to
# not break existing apps inside Google. Unlike Bazel, Google3 does not only
# enforce strict dependencies of source files, but also for generated files
# (such as the factory files). Therefore in order to avoid that generated files
# introduce new module dependencies (which aren't explicitly declared), we need
# to enable external symbol re-exports by default when running with Blaze.
"createExternalSymbolFactoryReexports": (not _is_bazel()),
# FIXME: wrong place to de-dupe
"expectedOut": depset([o.path for o in expected_outs]).to_list(),
}
@@ -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 {
@@ -942,6 +942,7 @@ function getAotCompilerOptions(options: CompilerOptions): AotCompilerOptions {
fullTemplateTypeCheck: options.fullTemplateTypeCheck,
allowEmptyCodegenFiles: options.allowEmptyCodegenFiles,
enableIvy: options.enableIvy,
createExternalSymbolFactoryReexports: options.createExternalSymbolFactoryReexports,
};
}

@@ -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,
@@ -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]
})
@@ -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);
@@ -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`
@@ -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
Oops, something went wrong.

0 comments on commit 91b7152

Please sign in to comment.
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.