Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(compiler): allow disabling external symbol factory reexports #28594

Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+78 −5
Diff settings

Always

Just for now

refactor(compiler): allow disabling external symbol factory reexports

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: #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: #25644 (comment)
  • Loading branch information...
devversion committed Feb 9, 2019
commit 883fb4b122236b80784b46911199414c7f2cc605
@@ -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, [
@@ -20,4 +20,5 @@ export interface AotCompilerOptions {
allowEmptyCodegenFiles?: boolean;
strictInjectionParameters?: boolean;
enableIvy?: boolean|'ngtsc'|'tsc';
createExternalSymbolFactoryReexports?: boolean;
}
@@ -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
@@ -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); });
@@ -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,
@@ -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}`;
@@ -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');
});
});
});
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.