Skip to content

Commit

Permalink
refactor(compiler): allow disabling external symbol factory reexports
Browse files Browse the repository at this point in the history
Currently external static symbols which are referenced by AOT
compiler generated code, will be re-exported in the corresponding
`.ngfactory` files.

This way of handling the symbol resolution has been introduced in
favor of avoding dynamically generated module dependencies. This
behavior therefore avoids any strict dependency failures.

Read more about a particular scenario here: angular#25644 (comment)

Now with `ngtsc`, this behavior has changed since `ngtsc` just
introduces these module dependencies in order to properly reference
the external symbol from its original location (also eliminating the need
for factories). Similarly we should provide a way to use the same
behavior with `ngc` because the downside of using the re-exported symbol
resolution is that user-code transformations (e.g. the `ngInjectableDef`
metadata which is added to the user source code), can resolve external
symbols to previous factory symbol re-exports. This is a critical issue
because it means that the actual JIT code references factory files in order
to access external symbols. This means that the generated output cannot
shipped to NPM without shipping the referenced factory files.

A specific example has been reported here: angular#25644 (comment)
  • Loading branch information
devversion committed Feb 9, 2019
1 parent 50732e1 commit 883fb4b
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 5 deletions.
2 changes: 1 addition & 1 deletion packages/compiler/src/aot/compiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,7 @@ export class AotCompiler {
null;
const {json, exportAs} = serializeSummaries(
srcFileName, forJitOutputCtx, this._summaryResolver, this._symbolResolver, symbolSummaries,
typeData);
typeData, this._options.createExternalSymbolFactoryReexports);
exportAs.forEach((entry) => {
ngFactoryCtx.statements.push(
o.variable(entry.exportAs).set(ngFactoryCtx.importExpr(entry.symbol)).toDeclStmt(null, [
Expand Down
1 change: 1 addition & 0 deletions packages/compiler/src/aot/compiler_options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,5 @@ export interface AotCompilerOptions {
allowEmptyCodegenFiles?: boolean;
strictInjectionParameters?: boolean;
enableIvy?: boolean|'ngtsc'|'tsc';
createExternalSymbolFactoryReexports?: boolean;
}
27 changes: 23 additions & 4 deletions packages/compiler/src/aot/summary_serializer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ export function serializeSummaries(
summary: CompileTypeSummary,
metadata: CompileNgModuleMetadata | CompileDirectiveMetadata | CompilePipeMetadata |
CompileTypeMetadata
}[]): {json: string, exportAs: {symbol: StaticSymbol, exportAs: string}[]} {
}[],
createExternalSymbolReexports =
true): {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 All @@ -36,7 +38,7 @@ export function serializeSummaries(
toJsonSerializer.addSummary(
{symbol: summary.type.reference, metadata: undefined, type: summary});
});
const {json, exportAs} = toJsonSerializer.serialize();
const {json, exportAs} = toJsonSerializer.serialize(createExternalSymbolReexports);
if (forJitCtx) {
const forJitSerializer = new ForJitSerializer(forJitCtx, symbolResolver, summaryResolver);
types.forEach(({summary, metadata}) => { forJitSerializer.addSourceType(summary, metadata); });
Expand Down Expand Up @@ -178,7 +180,14 @@ class ToJsonSerializer extends ValueTransformer {
}
}

serialize(): {json: string, exportAs: {symbol: StaticSymbol, exportAs: string}[]} {
/**
* @param createExternalSymbolReexports Whether external static symbols should be re-exported.
* This can be enabled if external symbols should be re-exported by the current module in
* order to avoid dynamically generated module dependencies which can break strict dependency
* enforcements (as in Google3). Read more here: https://github.com/angular/angular/issues/25644
*/
serialize(createExternalSymbolReexports: boolean):
{json: string, exportAs: {symbol: StaticSymbol, exportAs: string}[]} {
const exportAs: {symbol: StaticSymbol, exportAs: string}[] = [];
const json = JSON.stringify({
moduleName: this.moduleName,
Expand All @@ -189,8 +198,18 @@ class ToJsonSerializer extends ValueTransformer {
if (this.summaryResolver.isLibraryFile(symbol.filePath)) {
const reexportSymbol = this.reexportedBy.get(symbol);
if (reexportSymbol) {
// In case the given external static symbol is already manually exported by the
// user, we just proxy the external static symbol reference to the manual export.
// This ensures that the AOT compiler imports the external symbol through the
// user export and does not introduce another dependency which is not needed.
importAs = this.indexBySymbol.get(reexportSymbol) !;
} else {
} else if (createExternalSymbolReexports) {
// In this case, the given external static symbol is *not* manually exported by
// the user, and we manually create a re-export in the factory file so that we
// don't introduce another module dependency. This is useful when running within
// Bazel so that the AOT compiler does not introduce any module dependencies
// which can break the strict dependency enforcement. (e.g. as in Google3)
// Read more about this here: https://github.com/angular/angular/issues/25644
const summary = this.unprocessedSymbolSummariesBySymbol.get(symbol);
if (!summary || !summary.metadata || summary.metadata.__symbolic !== 'interface') {
importAs = `${symbol.name}_${index}`;
Expand Down
53 changes: 53 additions & 0 deletions packages/compiler/test/aot/summary_serializer_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -466,5 +466,58 @@ import {MockAotSummaryResolverHost, createMockOutputContext} from './summary_res
expect(serialized.json).not.toContain('error');
});
});

describe('createExternalSymbolReexports disabled', () => {
it('should use existing reexports for "importAs" for symbols of libraries', () => {
init();
const externalSerialized = serializeSummaries(
'someFile.ts', createMockOutputContext(), summaryResolver, symbolResolver,
[
{symbol: symbolCache.get('/tmp/external.ts', 'value'), metadata: 'aValue'},
{
symbol: symbolCache.get('/tmp/external.ts', 'reexportValue'),
metadata: symbolCache.get('/tmp/external.ts', 'value')
},
],
[], false);
expect(externalSerialized.exportAs).toEqual([]);
init({
'/tmp/external.ngsummary.json': externalSerialized.json,
});
const serialized = serializeSummaries(
'someFile.ts', createMockOutputContext(), summaryResolver, symbolResolver, [{
symbol: symbolCache.get('/tmp/test.ts', 'mainValue'),
metadata: symbolCache.get('/tmp/external.d.ts', 'reexportValue'),
}],
[]);
expect(serialized.exportAs).toEqual([]);
const importAs =
deserializeSummaries(symbolCache, summaryResolver, 'someFile.d.ts', serialized.json)
.importAs;
expect(importAs).toEqual([{
symbol: symbolCache.get('/tmp/external.d.ts', 'value'),
importAs: symbolCache.get('/tmp/test.d.ts', 'mainValue'),
}]);
});

it('should not create reexports in the ngfactory for external symbols', () => {
init();
const serialized = serializeSummaries(
'someFile.ts', createMockOutputContext(), summaryResolver, symbolResolver, [{
symbol: symbolCache.get('/tmp/test.ts', 'main'),
metadata: [
symbolCache.get('/tmp/external.d.ts', 'lib'),
symbolCache.get('/tmp/external.d.ts', 'lib', ['someMember']),
]
}],
[], false);
expect(serialized.exportAs.length)
.toBe(0, 'Expected no external symbols to be re-exported.');
const deserialized =
deserializeSummaries(symbolCache, summaryResolver, 'someFile.d.ts', serialized.json);
expect(deserialized.importAs.length)
.toBe(0, 'Expected no symbols that can be imported from a re-exported location');
});
});
});
}

0 comments on commit 883fb4b

Please sign in to comment.