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

fix(ivy): correctly write cross-file references (#25080) 

There is a bug in the existing handling for cross-file references.
Suppose there are two files, module.ts and component.ts.

component.ts declares two components, one of which uses the other.
In the Ivy model, this means the component will get a directives:
reference to the other in its defineComponent call.

That reference is generated by looking at the declared components
of the module (in module.ts). However, the way ngtsc tracks this
reference, it ends up comparing the identifier of the component
in module.ts with the component.ts file, detecting they're not in
the same file, and generating a relative import.

This commit changes ngtsc to track all identifiers of a reference,
including the one by which it is declared. This allows toExpression()
to correctly decide that a local reference is okay in component.ts.

PR Close #25080
This commit is contained in:
Alex Rickabaugh 2018-07-25 11:16:00 -07:00 committed by Igor Minar
parent ed7aa1c3e5
commit 13a0d527f6
2 changed files with 68 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

74
packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts
View File

@ -106,7 +106,7 @@ export abstract class Reference<T extends ts.Node = ts.Node> {
*/
abstract toExpression(context: ts.SourceFile, importMode?: ImportMode): Expression|null;
abstract withIdentifier(identifier: ts.Identifier): Reference;
abstract addIdentifier(identifier: ts.Identifier): void;
}
/**
@ -120,7 +120,7 @@ export class NodeReference<T extends ts.Node = ts.Node> extends Reference<T> {
toExpression(context: ts.SourceFile): null { return null; }
withIdentifier(identifier: ts.Identifier): NodeReference<T> { return this; }
addIdentifier(identifier: ts.Identifier): void {}
}
/**
@ -129,25 +129,18 @@ export class NodeReference<T extends ts.Node = ts.Node> extends Reference<T> {
* Imports generated by `ResolvedReference`s are always relative.
*/
export class ResolvedReference<T extends ts.Node = ts.Node> extends Reference<T> {
constructor(node: T, protected identifier: ts.Identifier) { super(node); }
protected identifiers: ts.Identifier[] = [];
constructor(node: T, protected primaryIdentifier: ts.Identifier) { super(node); }
readonly expressable = true;
toExpression(context: ts.SourceFile, importMode: ImportMode = ImportMode.UseExistingImport):
Expression {
let compareCtx: ts.Node|null = null;
switch (importMode) {
case ImportMode.UseExistingImport:
compareCtx = this.identifier;
break;
case ImportMode.ForceNewImport:
compareCtx = this.node;
break;
default:
throw new Error(`Unsupported ImportMode: ${ImportMode[importMode]}`);
}
if (ts.getOriginalNode(context) === ts.getOriginalNode(compareCtx).getSourceFile()) {
return new WrappedNodeExpr(this.identifier);
const localIdentifier =
pickIdentifier(context, this.primaryIdentifier, this.identifiers, importMode);
if (localIdentifier !== null) {
return new WrappedNodeExpr(localIdentifier);
} else {
// Relative import from context -> this.node.getSourceFile().
// TODO(alxhub): investigate the impact of multiple source roots here.
@ -165,16 +158,14 @@ export class ResolvedReference<T extends ts.Node = ts.Node> extends Reference<T>
// same.
if (relative === './') {
// Same file after all.
return new WrappedNodeExpr(this.identifier);
return new WrappedNodeExpr(this.primaryIdentifier);
} else {
return new ExternalExpr(new ExternalReference(relative, this.identifier.text));
return new ExternalExpr(new ExternalReference(relative, this.primaryIdentifier.text));
}
}
}
withIdentifier(identifier: ts.Identifier): ResolvedReference {
return new ResolvedReference(this.node, identifier);
}
addIdentifier(identifier: ts.Identifier): void { this.identifiers.push(identifier); }
}
/**
@ -184,8 +175,9 @@ export class ResolvedReference<T extends ts.Node = ts.Node> extends Reference<T>
* the module specifier will be an absolute module name, not a relative path.
*/
export class AbsoluteReference extends Reference {
private identifiers: ts.Identifier[] = [];
constructor(
node: ts.Node, private identifier: ts.Identifier, readonly moduleName: string,
node: ts.Node, private primaryIdentifier: ts.Identifier, readonly moduleName: string,
readonly symbolName: string) {
super(node);
}
@ -194,26 +186,29 @@ export class AbsoluteReference extends Reference {
toExpression(context: ts.SourceFile, importMode: ImportMode = ImportMode.UseExistingImport):
Expression {
let compareCtx: ts.Node|null = null;
switch (importMode) {
case ImportMode.UseExistingImport:
compareCtx = this.identifier;
break;
case ImportMode.ForceNewImport:
compareCtx = this.node;
break;
default:
throw new Error(`Unsupported ImportMode: ${ImportMode[importMode]}`);
}
if (ts.getOriginalNode(context) === ts.getOriginalNode(compareCtx).getSourceFile()) {
return new WrappedNodeExpr(this.identifier);
const localIdentifier =
pickIdentifier(context, this.primaryIdentifier, this.identifiers, importMode);
if (localIdentifier !== null) {
return new WrappedNodeExpr(localIdentifier);
} else {
return new ExternalExpr(new ExternalReference(this.moduleName, this.symbolName));
}
}
withIdentifier(identifier: ts.Identifier): AbsoluteReference {
return new AbsoluteReference(this.node, identifier, this.moduleName, this.symbolName);
addIdentifier(identifier: ts.Identifier): void { this.identifiers.push(identifier); }
}
function pickIdentifier(
context: ts.SourceFile, primary: ts.Identifier, secondaries: ts.Identifier[],
mode: ImportMode): ts.Identifier|null {
context = ts.getOriginalNode(context) as ts.SourceFile;
let localIdentifier: ts.Identifier|null = null;
if (ts.getOriginalNode(primary).getSourceFile() === context) {
return primary;
} else if (mode === ImportMode.UseExistingImport) {
return secondaries.find(id => ts.getOriginalNode(id).getSourceFile() === context) || null;
} else {
return null;
}
}
@ -426,10 +421,9 @@ class StaticInterpreter {
const result = this.visitDeclaration(
decl.node, {...context, absoluteModuleName: decl.viaModule || context.absoluteModuleName});
if (result instanceof Reference) {
return result.withIdentifier(node);
} else {
return result;
result.addIdentifier(node);
}
return result;
}
private visitDeclaration(node: ts.Declaration, context: Context): ResolvedValue {

34
packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
View File

@ -568,4 +568,38 @@ describe('ngtsc behavioral tests', () => {
expect(jsContents).toContain('i0.ɵd(dirIndex).onClick($event)');
expect(jsContents).toContain('i0.ɵd(dirIndex).onChange(i0.ɵd(dirIndex).arg)');
});
it('should correctly recognize local symbols', () => {
writeConfig();
write('module.ts', `
import {NgModule} from '@angular/core';
import {Dir, Comp} from './test';
@NgModule({
declarations: [Dir, Comp],
exports: [Dir, Comp],
})
class Module {}
`);
write(`test.ts`, `
import {Component, Directive} from '@angular/core';
@Directive({
selector: '[dir]',
})
export class Dir {}
@Component({
selector: 'test',
template: '<div dir>Test</div>',
})
export class Comp {}
`);
const exitCode = main(['-p', basePath], errorSpy);
expect(errorSpy).not.toHaveBeenCalled();
expect(exitCode).toBe(0);
const jsContents = getContents('test.js');
expect(jsContents).not.toMatch(/import \* as i[0-9] from ['"].\/test['"]/);
});
});