Skip to content

Commit

Permalink
fix(compiler-cli): handle nested qualified names in ctor injection in…
Browse files Browse the repository at this point in the history 
… local compilation mode

The current implementation assumes a qualified name consists of just two identifier, e.g., Foo.Bar. However it can be more nested, like Foo.Bar.Baz.XX.YY. While such nested patterns are quite uncommon and devs mostly just use two identifier here, the TS compiler seems to throw error if we make such assumption and it broke quite a lot of targets in g3 when compiled in local mode. So here we handle this nested property of qualified names.
  • Loading branch information
@pmvald
pmvald committed Sep 28, 2023
1 parent e1728a2 commit 72ca72e
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 22 deletions.
29 changes: 15 additions & 14 deletions packages/compiler-cli/src/ngtsc/annotations/common/src/di.ts
View file
Expand Up @@ -53,20 +53,12 @@ export function getConstructorDependencies(
const typeNode = param.typeValueReference.reason.typeNode;

if (ts.isTypeReferenceNode(typeNode)) {
if (ts.isIdentifier(typeNode.typeName)) {
token = new WrappedNodeExpr(typeNode.typeName);
} else if (ts.isQualifiedName(typeNode.typeName)) {
const receiver = new WrappedNodeExpr(typeNode.typeName.left);

// Here we manually create the token out of the typeName without caring about its
// references for better TS tracking. This is because in this code path the typeNode is
// imported from another file and since we are in local compilation mode (=single file
// mode) the reference of this node (or its typeName node) cannot be resolved. So all we
// can do is just to create a new expression.
token = new ReadPropExpr(receiver, typeNode.typeName.right.getText());
} else {
throw new Error('Impossible state!');
}
// Here we manually create the token out of the typeName without caring about its
// references for better TS tracking. This is because in this code path the typeNode is
// imported from another file and since we are in local compilation mode (=single file
// mode) the reference of this node (or its typeName node) cannot be resolved. So all we
// can do is just to create a new expression.
token = toQualifiedExpression(typeNode.typeName);
}
} else {
// In all other cases resolve the injection token
Expand Down Expand Up @@ -134,6 +126,15 @@ export function getConstructorDependencies(
}
}

/** Converts a TS qualified name to output expression. */
function toQualifiedExpression(entity: ts.EntityName): Expression {
if (ts.isIdentifier(entity)) {
return new WrappedNodeExpr(entity);
}

return new ReadPropExpr(toQualifiedExpression(entity.left), entity.right.getText());
}


/**
* Convert `ConstructorDeps` into the `R3DependencyMetadata` array for those deps if they're valid,
Expand Down
32 changes: 24 additions & 8 deletions packages/compiler-cli/test/ngtsc/local_compilation_spec.ts
View file
Expand Up @@ -475,6 +475,7 @@ runInEachFileSystem(() => {
import {SomeService1} from './some-where1'
import SomeService2 from './some-where2'
import * as SomeWhere3 from './some-where3'
import * as SomeWhere4 from './some-where4'
@Component({
selector: 'test-main',
Expand All @@ -485,6 +486,7 @@ runInEachFileSystem(() => {
private someService1: SomeService1,
private someService2: SomeService2,
private someService3: SomeWhere3.SomeService3,
private someService4: SomeWhere4.nested.SomeService4,
@Attribute('title') title: string,
@Inject(MESSAGE_TOKEN) tokenMessage: SomeClass,
) {}
Expand All @@ -502,7 +504,7 @@ runInEachFileSystem(() => {

expect(jsContents)
.toContain(
`MainComponent.ɵfac = function MainComponent_Factory(t) { return new (t || MainComponent)(i0.ɵɵdirectiveInject(SomeService1), i0.ɵɵdirectiveInject(SomeService2), i0.ɵɵdirectiveInject(SomeWhere3.SomeService3), i0.ɵɵinjectAttribute('title'), i0.ɵɵdirectiveInject(MESSAGE_TOKEN)); };`);
`MainComponent.ɵfac = function MainComponent_Factory(t) { return new (t || MainComponent)(i0.ɵɵdirectiveInject(SomeService1), i0.ɵɵdirectiveInject(SomeService2), i0.ɵɵdirectiveInject(SomeWhere3.SomeService3), i0.ɵɵdirectiveInject(SomeWhere4.nested.SomeService4), i0.ɵɵinjectAttribute('title'), i0.ɵɵdirectiveInject(MESSAGE_TOKEN)); };`);
});

it('should include injector types with all possible import/injection styles into standalone component factory',
Expand All @@ -513,6 +515,7 @@ runInEachFileSystem(() => {
import {SomeService1} from './some-where1'
import SomeService2 from './some-where2'
import * as SomeWhere3 from './some-where3'
import * as SomeWhere4 from './some-where4'
@Component({
standalone: true,
Expand All @@ -524,6 +527,7 @@ runInEachFileSystem(() => {
private someService1: SomeService1,
private someService2: SomeService2,
private someService3: SomeWhere3.SomeService3,
private someService4: SomeWhere4.nested.SomeService4,
@Attribute('title') title: string,
@Inject(MESSAGE_TOKEN) tokenMessage: SomeClass,
) {}
Expand All @@ -535,7 +539,7 @@ runInEachFileSystem(() => {

expect(jsContents)
.toContain(
`MainComponent.ɵfac = function MainComponent_Factory(t) { return new (t || MainComponent)(i0.ɵɵdirectiveInject(SomeService1), i0.ɵɵdirectiveInject(SomeService2), i0.ɵɵdirectiveInject(SomeWhere3.SomeService3), i0.ɵɵinjectAttribute('title'), i0.ɵɵdirectiveInject(MESSAGE_TOKEN)); };`);
`MainComponent.ɵfac = function MainComponent_Factory(t) { return new (t || MainComponent)(i0.ɵɵdirectiveInject(SomeService1), i0.ɵɵdirectiveInject(SomeService2), i0.ɵɵdirectiveInject(SomeWhere3.SomeService3), i0.ɵɵdirectiveInject(SomeWhere4.nested.SomeService4), i0.ɵɵinjectAttribute('title'), i0.ɵɵdirectiveInject(MESSAGE_TOKEN)); };`);
});

it('should include injector types with all possible import/injection styles into directive factory',
Expand All @@ -546,6 +550,7 @@ runInEachFileSystem(() => {
import {SomeService1} from './some-where1'
import SomeService2 from './some-where2'
import * as SomeWhere3 from './some-where3'
import * as SomeWhere4 from './some-where4'
@Directive({
})
Expand All @@ -554,6 +559,7 @@ runInEachFileSystem(() => {
private someService1: SomeService1,
private someService2: SomeService2,
private someService3: SomeWhere3.SomeService3,
private someService4: SomeWhere4.nested.SomeService4,
@Attribute('title') title: string,
@Inject(MESSAGE_TOKEN) tokenMessage: SomeClass,
) {}
Expand All @@ -571,7 +577,7 @@ runInEachFileSystem(() => {

expect(jsContents)
.toContain(
`MainDirective.ɵfac = function MainDirective_Factory(t) { return new (t || MainDirective)(i0.ɵɵdirectiveInject(SomeService1), i0.ɵɵdirectiveInject(SomeService2), i0.ɵɵdirectiveInject(SomeWhere3.SomeService3), i0.ɵɵinjectAttribute('title'), i0.ɵɵdirectiveInject(MESSAGE_TOKEN)); };`);
`MainDirective.ɵfac = function MainDirective_Factory(t) { return new (t || MainDirective)(i0.ɵɵdirectiveInject(SomeService1), i0.ɵɵdirectiveInject(SomeService2), i0.ɵɵdirectiveInject(SomeWhere3.SomeService3), i0.ɵɵdirectiveInject(SomeWhere4.nested.SomeService4), i0.ɵɵinjectAttribute('title'), i0.ɵɵdirectiveInject(MESSAGE_TOKEN)); };`);
});

it('should include injector types with all possible import/injection styles into standalone directive factory',
Expand All @@ -582,6 +588,7 @@ runInEachFileSystem(() => {
import {SomeService1} from './some-where1'
import SomeService2 from './some-where2'
import * as SomeWhere3 from './some-where3'
import * as SomeWhere4 from './some-where4'
@Directive({
standalone: true,
Expand All @@ -591,6 +598,7 @@ runInEachFileSystem(() => {
private someService1: SomeService1,
private someService2: SomeService2,
private someService3: SomeWhere3.SomeService3,
private someService4: SomeWhere4.nested.SomeService4,
@Attribute('title') title: string,
@Inject(MESSAGE_TOKEN) tokenMessage: SomeClass,
) {}
Expand All @@ -602,7 +610,7 @@ runInEachFileSystem(() => {

expect(jsContents)
.toContain(
`MainDirective.ɵfac = function MainDirective_Factory(t) { return new (t || MainDirective)(i0.ɵɵdirectiveInject(SomeService1), i0.ɵɵdirectiveInject(SomeService2), i0.ɵɵdirectiveInject(SomeWhere3.SomeService3), i0.ɵɵinjectAttribute('title'), i0.ɵɵdirectiveInject(MESSAGE_TOKEN)); };`);
`MainDirective.ɵfac = function MainDirective_Factory(t) { return new (t || MainDirective)(i0.ɵɵdirectiveInject(SomeService1), i0.ɵɵdirectiveInject(SomeService2), i0.ɵɵdirectiveInject(SomeWhere3.SomeService3), i0.ɵɵdirectiveInject(SomeWhere4.nested.SomeService4), i0.ɵɵinjectAttribute('title'), i0.ɵɵdirectiveInject(MESSAGE_TOKEN)); };`);
});

it('should include injector types with all possible import/injection styles into pipe factory',
Expand All @@ -613,13 +621,15 @@ runInEachFileSystem(() => {
import {SomeService1} from './some-where1'
import SomeService2 from './some-where2'
import * as SomeWhere3 from './some-where3'
import * as SomeWhere4 from './some-where4'
@Pipe({name: 'pipe'})
export class MainPipe {
constructor(
private someService1: SomeService1,
private someService2: SomeService2,
private someService3: SomeWhere3.SomeService3,
private someService4: SomeWhere4.nested.SomeService4,
@Attribute('title') title: string,
@Inject(MESSAGE_TOKEN) tokenMessage: SomeClass,
) {}
Expand All @@ -637,7 +647,7 @@ runInEachFileSystem(() => {

expect(jsContents)
.toContain(
`MainPipe.ɵfac = function MainPipe_Factory(t) { return new (t || MainPipe)(i0.ɵɵdirectiveInject(SomeService1, 16), i0.ɵɵdirectiveInject(SomeService2, 16), i0.ɵɵdirectiveInject(SomeWhere3.SomeService3, 16), i0.ɵɵinjectAttribute('title'), i0.ɵɵdirectiveInject(MESSAGE_TOKEN, 16)); };`);
`MainPipe.ɵfac = function MainPipe_Factory(t) { return new (t || MainPipe)(i0.ɵɵdirectiveInject(SomeService1, 16), i0.ɵɵdirectiveInject(SomeService2, 16), i0.ɵɵdirectiveInject(SomeWhere3.SomeService3, 16), i0.ɵɵdirectiveInject(SomeWhere4.nested.SomeService4, 16), i0.ɵɵinjectAttribute('title'), i0.ɵɵdirectiveInject(MESSAGE_TOKEN, 16)); };`);
});

it('should include injector types with all possible import/injection styles into standalone pipe factory',
Expand All @@ -648,6 +658,7 @@ runInEachFileSystem(() => {
import {SomeService1} from './some-where1'
import SomeService2 from './some-where2'
import * as SomeWhere3 from './some-where3'
import * as SomeWhere4 from './some-where4'
@Pipe({
name: 'pipe',
Expand All @@ -658,6 +669,7 @@ runInEachFileSystem(() => {
private someService1: SomeService1,
private someService2: SomeService2,
private someService3: SomeWhere3.SomeService3,
private someService4: SomeWhere4.nested.SomeService4,
@Attribute('title') title: string,
@Inject(MESSAGE_TOKEN) tokenMessage: SomeClass,
) {}
Expand All @@ -669,7 +681,7 @@ runInEachFileSystem(() => {

expect(jsContents)
.toContain(
`MainPipe.ɵfac = function MainPipe_Factory(t) { return new (t || MainPipe)(i0.ɵɵdirectiveInject(SomeService1, 16), i0.ɵɵdirectiveInject(SomeService2, 16), i0.ɵɵdirectiveInject(SomeWhere3.SomeService3, 16), i0.ɵɵinjectAttribute('title'), i0.ɵɵdirectiveInject(MESSAGE_TOKEN, 16)); };`);
`MainPipe.ɵfac = function MainPipe_Factory(t) { return new (t || MainPipe)(i0.ɵɵdirectiveInject(SomeService1, 16), i0.ɵɵdirectiveInject(SomeService2, 16), i0.ɵɵdirectiveInject(SomeWhere3.SomeService3, 16), i0.ɵɵdirectiveInject(SomeWhere4.nested.SomeService4, 16), i0.ɵɵinjectAttribute('title'), i0.ɵɵdirectiveInject(MESSAGE_TOKEN, 16)); };`);
});

it('should include injector types with all possible import/injection styles into injectable factory',
Expand All @@ -680,6 +692,7 @@ runInEachFileSystem(() => {
import {SomeService1} from './some-where1'
import SomeService2 from './some-where2'
import * as SomeWhere3 from './some-where3'
import * as SomeWhere4 from './some-where4'
@Injectable({
providedIn: 'root',
Expand All @@ -689,6 +702,7 @@ runInEachFileSystem(() => {
private someService1: SomeService1,
private someService2: SomeService2,
private someService3: SomeWhere3.SomeService3,
private someService4: SomeWhere4.nested.SomeService4,
@Attribute('title') title: string,
@Inject(MESSAGE_TOKEN) tokenMessage: SomeClass,
) {}
Expand All @@ -700,7 +714,7 @@ runInEachFileSystem(() => {

expect(jsContents)
.toContain(
`MainService.ɵfac = function MainService_Factory(t) { return new (t || MainService)(i0.ɵɵinject(SomeService1), i0.ɵɵinject(SomeService2), i0.ɵɵinject(SomeWhere3.SomeService3), i0.ɵɵinjectAttribute('title'), i0.ɵɵinject(MESSAGE_TOKEN)); };`);
`MainService.ɵfac = function MainService_Factory(t) { return new (t || MainService)(i0.ɵɵinject(SomeService1), i0.ɵɵinject(SomeService2), i0.ɵɵinject(SomeWhere3.SomeService3), i0.ɵɵinject(SomeWhere4.nested.SomeService4), i0.ɵɵinjectAttribute('title'), i0.ɵɵinject(MESSAGE_TOKEN)); };`);
});

it('should include injector types with all possible import/injection styles into ng module factory',
Expand All @@ -711,6 +725,7 @@ runInEachFileSystem(() => {
import {SomeService1} from './some-where1'
import SomeService2 from './some-where2'
import * as SomeWhere3 from './some-where3'
import * as SomeWhere4 from './some-where4'
@NgModule({
})
Expand All @@ -719,6 +734,7 @@ runInEachFileSystem(() => {
private someService1: SomeService1,
private someService2: SomeService2,
private someService3: SomeWhere3.SomeService3,
private someService4: SomeWhere4.nested.SomeService4,
@Attribute('title') title: string,
@Inject(MESSAGE_TOKEN) tokenMessage: SomeClass,
) {}
Expand All @@ -730,7 +746,7 @@ runInEachFileSystem(() => {

expect(jsContents)
.toContain(
`MainModule.ɵfac = function MainModule_Factory(t) { return new (t || MainModule)(i0.ɵɵinject(SomeService1), i0.ɵɵinject(SomeService2), i0.ɵɵinject(SomeWhere3.SomeService3), i0.ɵɵinjectAttribute('title'), i0.ɵɵinject(MESSAGE_TOKEN)); };`);
`MainModule.ɵfac = function MainModule_Factory(t) { return new (t || MainModule)(i0.ɵɵinject(SomeService1), i0.ɵɵinject(SomeService2), i0.ɵɵinject(SomeWhere3.SomeService3), i0.ɵɵinject(SomeWhere4.nested.SomeService4), i0.ɵɵinjectAttribute('title'), i0.ɵɵinject(MESSAGE_TOKEN)); };`);
});
});

Expand Down

0 comments on commit 72ca72e

Please sign in to comment.