iSharkFly-Docs
/
angular-docs-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(compiler-cli): prefer non-aliased exports in reference emitters (#41866) 

This commit changes the reference emitters in the Ivy compiler to prefer
non-aliased exports if they exist. This avoids selecting "private
exports" that may not be stable, e.g. the reexports that have been added
by the View Engine compiler. Such reexports are not stable and are
therefore not suitable to be emitted into partial compilations, as the
output of partial compilations should only reference stable symbols
from upstream libraries.

An alternative solution has been considered where ViewEngine-generated
exports would gain a certain prefix, such that the Ivy compiler could
just exclude those exports (see #41443). However, that solution would
be insufficient in case a library is built using partial compilation and
while depending itself on a VE-compiled library from earlier versions of
Angular, where the magic prefix would be missing. For such libraries,
ngcc would have generated reexports using the declared name if not already
present so this change does result in choosing the correct export.

Because ngcc always generates reexports using the declared name even if
an aliased export is present, this change causes those ngcc-generated
exports to be chosen in downstream libraries using partial compilation.
This is unfortunate as it means that the declared names become
effectively public even if the library author was intentionally
exporting it using an alias. This commit does not address this problem;
it is expected that this should not result in widespread issues across
the library ecosystem.

Fixes #41277

PR Close #41866
This commit is contained in:
JoostK 2021-04-28 22:14:38 +02:00 committed by Misko Hevery
parent 2cc73526bc
commit 35450c78f7
11 changed files with 295 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
packages/compiler-cli/src/ngtsc/imports/src/emitter.ts
View File

@ -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<DeclarationNode, string>();
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};
}
}

28
packages/compiler-cli/src/ngtsc/imports/src/find_export.ts
View File

@ -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;
}
/**

1
packages/compiler-cli/src/ngtsc/imports/test/BUILD.bazel
View File

@ -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",

143
packages/compiler-cli/src/ngtsc/imports/test/emitter_spec.ts
View File

@ -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');
});
});
});

5
packages/compiler-cli/src/ngtsc/util/src/typescript.ts
View File

@ -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)) {

52
packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/GOLDEN_PARTIAL.js
View File

@ -567,3 +567,55 @@ export declare class TestCmp {
static ɵcmp: i0.ɵɵComponentDeclaration<TestCmp, "test-cmp", never, {}, {}, never, never>;
}
/****************************************************************************************************
* 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: `
<lib-dir></lib-dir>
`, 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: `
<lib-dir></lib-dir>
`,
}]
}] });
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<TestComponent, never>;
static ɵcmp: i0.ɵɵComponentDeclaration<TestComponent, "ng-component", never, {}, {}, never, never>;
}
export declare class TestModule {
static ɵfac: i0.ɵɵFactoryDeclaration<TestModule, never>;
static ɵmod: i0.ɵɵNgModuleDeclaration<TestModule, [typeof TestComponent], [typeof i1.LibModule], never>;
static ɵinj: i0.ɵɵInjectorDeclaration<TestModule>;
}

21
packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/TEST_CASES.json
View File

@ -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"]
}
}
}
]
}

16
packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/external_library.d.ts vendored Normal file
View File

@ -0,0 +1,16 @@
import * as ɵngcc0 from '@angular/core';
declare class LibDirective {
static ɵfac: ɵngcc0.ɵɵFactoryDeclaration<LibDirective, never>;
static ɵdir: ɵngcc0.ɵɵDirectiveDeclaration<LibDirective, 'lib-dir', never, {}, {}, never>;
}
export declare class LibModule {
static ɵfac: ɵngcc0.ɵɵFactoryDeclaration<LibModule, never>;
static ɵmod: ɵngcc0.ɵɵNgModuleDeclaration<LibModule, [typeof LibDirective], never, [typeof LibDirective]>;
static ɵinj: ɵngcc0.ɵɵInjectorDeclaration<LibModule>;
}
export {LibDirective as ɵangular_packages_forms_forms_a}
export {LibDirective}
export {LibDirective as ɵangular_packages_forms_forms_b}

3
packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/library_exports.js Normal file
View File

@ -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]

20
packages/compiler-cli/test/compliance/test_cases/r3_compiler_compliance/components_and_directives/library_exports.ts Normal file
View File

@ -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: `
<lib-dir></lib-dir>
`,
})
export class TestComponent {
}
@NgModule({
declarations: [TestComponent],
imports: [LibModule],
})
export class TestModule {
}

4
packages/compiler-cli/test/ngtsc/incremental_semantic_changes_spec.ts
View File

@ -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';