fix(compiler-cli): handle nested qualified names in ctor injection in… #51947
Conversation
pmvald
added
action: review
| @@ -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)); };`); | |||
There was a problem hiding this comment.
OOC: Do you know why/if the import to SomeWhere4 is actually preserved? IIRC TypeScript would SomeWhere4 because it's not used as a "value" anywhere (TS analyzes this before the transform).
there is also a special case with default imports. e.g. if SomeWhere4 would be a namespace import: https://github.com/angular/angular/blob/main/packages/compiler-cli/src/ngtsc/imports/src/default.ts#L40-L69
There was a problem hiding this comment.
Trying this on some real ts files, I see all imports are preserved. I guess since this transformation is a "before" transformation then TS does the removal analysis after it, in which case the identifier is now used as a value in ɵɵdirectiveInject ??? At least this is what I expect from a "before" transformation. But I heard Doug was talking about earlier elision of types, even before the transformation. I did not experience this, and as you see ctor injection works fine even cross g3 without any issue related to type removal.
There was a problem hiding this comment.
For the default import, I did not include this case in the tests since g3 does not allow it. But in my local testing it works. Basically the import remains the same and whatever appears as type will appear inside ɵɵdirectiveInject as it is.
… 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.
pmvald
force-pushed
the
lcm-qualified-name
branch
from
72ca72e
to
1925259
Compare
pmvald
added
action: merge
|
This PR was merged into the repository by commit 11bb19c. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
… local compilation mode (angular#51947) 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. PR Close angular#51947
… 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.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information