diff --git a/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts b/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts index f21dca0ce1..80b2bf95b1 100644 --- a/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts +++ b/packages/compiler-cli/src/ngtsc/imports/src/emitter.ts @@ -12,7 +12,7 @@ import {UnifiedModulesHost} from '../../core/api'; import {absoluteFromSourceFile, dirname, LogicalFileSystem, LogicalProjectPath, relative, toRelativeImport} from '../../file_system'; import {stripExtension} from '../../file_system/src/util'; import {DeclarationNode, ReflectionHost} from '../../reflection'; -import {getSourceFile, isDeclaration, isTypeDeclaration, nodeNameForError} from '../../util/src/typescript'; +import {getSourceFile, isDeclaration, isNamedDeclaration, isTypeDeclaration, nodeNameForError} from '../../util/src/typescript'; import {findExportedNameOfNode} from './find_export'; import {Reference} from './references'; @@ -255,9 +255,20 @@ export class AbsoluteModuleStrategy implements ReferenceEmitStrategy { return null; } const exportMap = new Map(); - exports.forEach((declaration, name) => { + for (const [name, declaration] of exports) { + if (exportMap.has(declaration.node)) { + // An export for this declaration has already been registered. We prefer an export that + // has the same name as the declared name, i.e. is not an aliased export. This is relevant + // for partial compilations where emitted references should import symbols using a stable + // name. This is particularly relevant for declarations inside VE-generated libraries, as + // such libraries contain private, unstable reexports of symbols. + const existingExport = exportMap.get(declaration.node)!; + if (isNamedDeclaration(declaration.node) && declaration.node.name.text === existingExport) { + continue; + } + } exportMap.set(declaration.node, name); - }); + } return {module: entryPointFile, exportMap}; } } diff --git a/packages/compiler-cli/src/ngtsc/imports/src/find_export.ts b/packages/compiler-cli/src/ngtsc/imports/src/find_export.ts index e514ae7a9d..3809845f6e 100644 --- a/packages/compiler-cli/src/ngtsc/imports/src/find_export.ts +++ b/packages/compiler-cli/src/ngtsc/imports/src/find_export.ts @@ -8,6 +8,7 @@ import * as ts from 'typescript'; import {ReflectionHost} from '../../reflection'; +import {isNamedDeclaration} from '../../util/src/typescript'; /** * Find the name, if any, by which a node is exported from a given file. @@ -18,18 +19,29 @@ export function findExportedNameOfNode( if (exports === null) { return null; } - // Look for the export which declares the node. - const keys = Array.from(exports.keys()); - const name = keys.find(key => { - const decl = exports.get(key); - return decl !== undefined && decl.node === target; - }); - if (name === undefined) { + const declaredName = isNamedDeclaration(target) ? target.name.text : null; + + // Look for the export which declares the node. + let foundExportName: string|null = null; + for (const [exportName, declaration] of exports) { + if (declaration.node !== target) { + continue; + } + + if (exportName === declaredName) { + // A non-alias export exists which is always preferred, so use that one. + return exportName; + } + + foundExportName = exportName; + } + + if (foundExportName === null) { throw new Error( `Failed to find exported name of node (${target.getText()}) in '${file.fileName}'.`); } - return name; + return foundExportName; } /** diff --git a/packages/compiler-cli/src/ngtsc/imports/test/BUILD.bazel b/packages/compiler-cli/src/ngtsc/imports/test/BUILD.bazel index 44f7f734c1..569b513b7a 100644 --- a/packages/compiler-cli/src/ngtsc/imports/test/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/imports/test/BUILD.bazel @@ -11,6 +11,7 @@ ts_library( deps = [ "//packages:types", "//packages/compiler", + "//packages/compiler-cli/src/ngtsc/core:api", "//packages/compiler-cli/src/ngtsc/file_system", "//packages/compiler-cli/src/ngtsc/file_system/testing", "//packages/compiler-cli/src/ngtsc/imports", diff --git a/packages/compiler-cli/src/ngtsc/imports/test/emitter_spec.ts b/packages/compiler-cli/src/ngtsc/imports/test/emitter_spec.ts index c556a1cf72..f38190865f 100644 --- a/packages/compiler-cli/src/ngtsc/imports/test/emitter_spec.ts +++ b/packages/compiler-cli/src/ngtsc/imports/test/emitter_spec.ts @@ -7,12 +7,13 @@ */ import {ExternalExpr} from '@angular/compiler'; import * as ts from 'typescript'; +import {UnifiedModulesHost} from '../../core/api'; -import {absoluteFrom as _, LogicalFileSystem} from '../../file_system'; +import {absoluteFrom as _, basename, LogicalFileSystem} from '../../file_system'; import {runInEachFileSystem, TestFile} from '../../file_system/testing'; import {Declaration, TypeScriptReflectionHost} from '../../reflection'; import {getDeclaration, makeProgram} from '../../testing'; -import {AbsoluteModuleStrategy, ImportFlags, LogicalProjectStrategy} from '../src/emitter'; +import {AbsoluteModuleStrategy, ImportFlags, LogicalProjectStrategy, RelativePathStrategy, UnifiedModulesStrategy} from '../src/emitter'; import {Reference} from '../src/references'; import {ModuleResolver} from '../src/resolver'; @@ -49,6 +50,41 @@ runInEachFileSystem(() => { expect(emitted).toBeNull(); }); + it('should prefer non-aliased exports', () => { + const {strategy, program} = makeStrategy([ + { + name: _('/node_modules/external.d.ts'), + contents: ` + declare class Foo {} + export {Foo as A}; + export {Foo}; + export {Foo as B}; + `, + }, + { + name: _('/context.ts'), + contents: 'export class Context {}', + }, + ]); + const decl = + getDeclaration(program, _('/node_modules/external.d.ts'), 'Foo', ts.isClassDeclaration); + const context = program.getSourceFile(_('/context.ts'))!; + + const reference = new Reference(decl, { + specifier: 'external', + resolutionContext: context.fileName, + }); + const emitted = strategy.emit(reference, context, ImportFlags.None); + if (emitted === null) { + return fail('Reference should be emitted'); + } + if (!(emitted.expression instanceof ExternalExpr)) { + return fail('Reference should be emitted as ExternalExpr'); + } + expect(emitted.expression.value.name).toEqual('Foo'); + expect(emitted.expression.value.moduleName).toEqual('external'); + }); + it('should generate an import using the exported name of the declaration', () => { const {strategy, program} = makeStrategy([ { @@ -173,5 +209,108 @@ runInEachFileSystem(() => { // Expect the prefixed name from the TestHost. expect((ref!.expression as ExternalExpr).value.name).toEqual('testFoo'); }); + + it('should prefer non-aliased exports', () => { + const {program, host} = makeProgram([ + { + name: _('/index.ts'), + contents: ` + declare class Foo {} + export {Foo as A}; + export {Foo}; + export {Foo as B}; + `, + }, + { + name: _('/context.ts'), + contents: 'export class Context {}', + } + ]); + const checker = program.getTypeChecker(); + const logicalFs = new LogicalFileSystem([_('/')], host); + const strategy = new LogicalProjectStrategy(new TypeScriptReflectionHost(checker), logicalFs); + const decl = getDeclaration(program, _('/index.ts'), 'Foo', ts.isClassDeclaration); + const context = program.getSourceFile(_('/context.ts'))!; + const emitted = strategy.emit(new Reference(decl), context); + if (emitted === null) { + return fail('Reference should be emitted'); + } + if (!(emitted.expression instanceof ExternalExpr)) { + return fail('Reference should be emitted as ExternalExpr'); + } + expect(emitted.expression.value.name).toEqual('Foo'); + expect(emitted.expression.value.moduleName).toEqual('./index'); + }); + }); + + describe('RelativePathStrategy', () => { + it('should prefer non-aliased exports', () => { + const {program} = makeProgram([ + { + name: _('/index.ts'), + contents: ` + declare class Foo {} + export {Foo as A}; + export {Foo}; + export {Foo as B}; + `, + }, + { + name: _('/context.ts'), + contents: 'export class Context {}', + } + ]); + const checker = program.getTypeChecker(); + const strategy = new RelativePathStrategy(new TypeScriptReflectionHost(checker)); + const decl = getDeclaration(program, _('/index.ts'), 'Foo', ts.isClassDeclaration); + const context = program.getSourceFile(_('/context.ts'))!; + const emitted = strategy.emit(new Reference(decl), context); + if (emitted === null) { + return fail('Reference should be emitted'); + } + if (!(emitted.expression instanceof ExternalExpr)) { + return fail('Reference should be emitted as ExternalExpr'); + } + expect(emitted.expression.value.name).toEqual('Foo'); + expect(emitted.expression.value.moduleName).toEqual('./index'); + }); + }); + + describe('UnifiedModulesStrategy', () => { + it('should prefer non-aliased exports', () => { + const {program} = makeProgram([ + { + name: _('/index.ts'), + contents: ` + declare class Foo {} + export {Foo as A}; + export {Foo}; + export {Foo as B}; + `, + }, + { + name: _('/context.ts'), + contents: 'export class Context {}', + } + ]); + const checker = program.getTypeChecker(); + const host: UnifiedModulesHost = { + fileNameToModuleName(importedFilePath): string { + return basename(importedFilePath, '.ts'); + } + }; + const strategy = new UnifiedModulesStrategy(new TypeScriptReflectionHost(checker), host); + const decl = getDeclaration(program, _('/index.ts'), 'Foo', ts.isClassDeclaration); + const context = program.getSourceFile(_('/context.ts'))!; + const emitted = strategy.emit(new Reference(decl), context); + if (emitted === null) { + return fail('Reference should be emitted'); + } + if (!(emitted.expression instanceof ExternalExpr)) { + return fail('Reference should be emitted as ExternalExpr'); + } + expect(emitted.expression.value.name).toEqual('Foo'); + expect(emitted.expression.value.moduleName).toEqual('index'); + }); }); }); diff --git a/packages/compiler-cli/src/ngtsc/util/src/typescript.ts b/packages/compiler-cli/src/ngtsc/util/src/typescript.ts index 565fab3026..44854f778b 100644 --- a/packages/compiler-cli/src/ngtsc/util/src/typescript.ts +++ b/packages/compiler-cli/src/ngtsc/util/src/typescript.ts @@ -83,6 +83,11 @@ export function isTypeDeclaration(node: ts.Node): node is ts.EnumDeclaration| ts.isInterfaceDeclaration(node); } +export function isNamedDeclaration(node: ts.Node): node is ts.Declaration&{name: ts.Identifier} { + const namedNode = node as {name?: ts.Identifier}; + return namedNode.name !== undefined && ts.isIdentifier(namedNode.name); +} + export function isExported(node: DeclarationNode): boolean { let topLevel: ts.Node = node; if (ts.isVariableDeclaration(node) && ts.isVariableDeclarationList(node.parent)) { diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/GOLDEN_PARTIAL.js b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/GOLDEN_PARTIAL.js index c4d6e1f421..ec7381e4d5 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/GOLDEN_PARTIAL.js +++ b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/GOLDEN_PARTIAL.js @@ -567,3 +567,55 @@ export declare class TestCmp { static ɵcmp: i0.ɵɵComponentDeclaration; } +/**************************************************************************************************** + * PARTIAL FILE: library_exports.js + ****************************************************************************************************/ +// This test verifies that a directive from an external library is emitted using its declared name, +// even in the presence of alias exports that could have been chosen. +// See https://github.com/angular/angular/issues/41277. +import { Component, NgModule } from '@angular/core'; +import { LibModule } from 'external_library'; +import * as i0 from "@angular/core"; +import * as i1 from "external_library"; +export class TestComponent { +} +TestComponent.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: TestComponent, deps: [], target: i0.ɵɵFactoryTarget.Component }); +TestComponent.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", type: TestComponent, selector: "ng-component", ngImport: i0, template: ` + + `, isInline: true, directives: [{ type: i1.LibDirective, selector: "lib-dir" }] }); +i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: TestComponent, decorators: [{ + type: Component, + args: [{ + template: ` + + `, + }] + }] }); +export class TestModule { +} +TestModule.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: TestModule, deps: [], target: i0.ɵɵFactoryTarget.NgModule }); +TestModule.ɵmod = i0.ɵɵngDeclareNgModule({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: TestModule, declarations: [TestComponent], imports: [LibModule] }); +TestModule.ɵinj = i0.ɵɵngDeclareInjector({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: TestModule, imports: [[LibModule]] }); +i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: TestModule, decorators: [{ + type: NgModule, + args: [{ + declarations: [TestComponent], + imports: [LibModule], + }] + }] }); + +/**************************************************************************************************** + * PARTIAL FILE: library_exports.d.ts + ****************************************************************************************************/ +import * as i0 from "@angular/core"; +import * as i1 from "external_library"; +export declare class TestComponent { + static ɵfac: i0.ɵɵFactoryDeclaration; + static ɵcmp: i0.ɵɵComponentDeclaration; +} +export declare class TestModule { + static ɵfac: i0.ɵɵFactoryDeclaration; + static ɵmod: i0.ɵɵNgModuleDeclaration; + static ɵinj: i0.ɵɵInjectorDeclaration; +} + diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/TEST_CASES.json b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/TEST_CASES.json index 0e4f3454d7..168e866fce 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/TEST_CASES.json +++ b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/TEST_CASES.json @@ -178,6 +178,27 @@ "inputFiles": [ "non_literal_template_with_concatenation.ts" ] + }, + { + "description": "should not use reexport names inside declarations when a direct export is available", + "inputFiles": [ + "external_library.d.ts", + "library_exports.ts" + ], + "expectations": [ + { + "failureMessage": "Invalid list of directives", + "files": [ + "library_exports.js" + ] + } + ], + "compilerOptions": { + "baseUrl": ".", + "paths": { + "external_library": ["./external_library"] + } + } } ] } diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/external_library.d.ts b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/external_library.d.ts new file mode 100644 index 0000000000..dc662e9081 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/external_library.d.ts @@ -0,0 +1,16 @@ +import * as ɵngcc0 from '@angular/core'; + +declare class LibDirective { + static ɵfac: ɵngcc0.ɵɵFactoryDeclaration; + static ɵdir: ɵngcc0.ɵɵDirectiveDeclaration; +} + +export declare class LibModule { + static ɵfac: ɵngcc0.ɵɵFactoryDeclaration; + static ɵmod: ɵngcc0.ɵɵNgModuleDeclaration; + static ɵinj: ɵngcc0.ɵɵInjectorDeclaration; +} + +export {LibDirective as ɵangular_packages_forms_forms_a} +export {LibDirective} +export {LibDirective as ɵangular_packages_forms_forms_b} diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/library_exports.js b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/library_exports.js new file mode 100644 index 0000000000..91fa9baf35 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/library_exports.js @@ -0,0 +1,3 @@ +// NOTE: The declared name must have been used to refer to `LibDirective`, as the aliased exports +// NOTE: are not stable and therefore not suitable for partial compilation outputs. +directives: [$i0$.LibDirective] diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/library_exports.ts b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/library_exports.ts new file mode 100644 index 0000000000..4f643ab12a --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/library_exports.ts @@ -0,0 +1,20 @@ +// This test verifies that a directive from an external library is emitted using its declared name, +// even in the presence of alias exports that could have been chosen. +// See https://github.com/angular/angular/issues/41277. +import {Component, NgModule} from '@angular/core'; +import {LibModule} from 'external_library'; + +@Component({ + template: ` + + `, +}) +export class TestComponent { +} + +@NgModule({ + declarations: [TestComponent], + imports: [LibModule], +}) +export class TestModule { +} diff --git a/packages/compiler-cli/test/ngtsc/incremental_semantic_changes_spec.ts b/packages/compiler-cli/test/ngtsc/incremental_semantic_changes_spec.ts index 345ed61911..c6b466c5b3 100644 --- a/packages/compiler-cli/test/ngtsc/incremental_semantic_changes_spec.ts +++ b/packages/compiler-cli/test/ngtsc/incremental_semantic_changes_spec.ts @@ -1217,7 +1217,7 @@ runInEachFileSystem(() => { selector: 'cmp-dep', template: 'Dep', }) - export class CmpDep {} + class CmpDep {} `); env.write('module.ts', ` import {NgModule} from '@angular/core'; @@ -1246,7 +1246,7 @@ runInEachFileSystem(() => { selector: 'cmp-dep', template: 'Dep', }) - export class CmpDep {} + class CmpDep {} `); env.write('module.ts', ` import {NgModule} from '@angular/core';