From 872a3656fedeba2d786d12c0fd57c19cb93f67db Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sat, 9 Feb 2019 19:13:18 +0100 Subject: [PATCH] refactor(compiler): allow disabling external symbol factory reexports (#28594) 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: https://github.com/angular/angular/issues/25644#issuecomment-458354439 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: https://github.com/angular/angular/issues/25644#issue-353554070 PR Close #28594 --- packages/compiler/src/aot/compiler.ts | 2 +- packages/compiler/src/aot/compiler_options.ts | 1 + .../compiler/src/aot/summary_serializer.ts | 27 ++++++++-- .../test/aot/summary_serializer_spec.ts | 53 +++++++++++++++++++ 4 files changed, 78 insertions(+), 5 deletions(-) diff --git a/packages/compiler/src/aot/compiler.ts b/packages/compiler/src/aot/compiler.ts index f76e0fe06c..f7b5c59e25 100644 --- a/packages/compiler/src/aot/compiler.ts +++ b/packages/compiler/src/aot/compiler.ts @@ -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, [ diff --git a/packages/compiler/src/aot/compiler_options.ts b/packages/compiler/src/aot/compiler_options.ts index f8fbdf2800..4a0e3f9532 100644 --- a/packages/compiler/src/aot/compiler_options.ts +++ b/packages/compiler/src/aot/compiler_options.ts @@ -20,4 +20,5 @@ export interface AotCompilerOptions { allowEmptyCodegenFiles?: boolean; strictInjectionParameters?: boolean; enableIvy?: boolean|'ngtsc'|'tsc'; + createExternalSymbolFactoryReexports?: boolean; } diff --git a/packages/compiler/src/aot/summary_serializer.ts b/packages/compiler/src/aot/summary_serializer.ts index 1b6d44885d..ae63e6cf0a 100644 --- a/packages/compiler/src/aot/summary_serializer.ts +++ b/packages/compiler/src/aot/summary_serializer.ts @@ -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}`; diff --git a/packages/compiler/test/aot/summary_serializer_spec.ts b/packages/compiler/test/aot/summary_serializer_spec.ts index 26cae7dd97..0c89e4b7c8 100644 --- a/packages/compiler/test/aot/summary_serializer_spec.ts +++ b/packages/compiler/test/aot/summary_serializer_spec.ts @@ -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'); + }); + }); }); }