Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(compiler-cli): properly emit literal types when recreating type parameters in a different file #42761

Closed

Conversation

JoostK
Copy link
Member

@JoostK JoostK commented Jul 3, 2021

In #42492 the template type checker became capable of replicating a
wider range of generic type parameters for use in template type-check
files. Any literal types within a type parameter would however emit
invalid code, as TypeScript was emitting the literals using the text as
extracted from the template type-check file instead of the original
source file where the type node was taken from.

This commit works around the issue by cloning any literal types and
marking them as synthetic, signalling to TypeScript that the literal
text has to be extracted from the node itself instead from the source
file.

This commit also excludes import() type nodes from being supported,
as their module specifier may potentially need to be rewritten.

Fixes #42667

@JoostK JoostK added target: patch This PR is targeted for the next patch release area: compiler Issues related to `ngc`, Angular's template compiler P2 The issue is important to a large percentage of users, with a workaround labels Jul 3, 2021
@ngbot ngbot bot modified the milestone: Backlog Jul 3, 2021
@google-cla google-cla bot added the cla: yes label Jul 3, 2021
@JoostK JoostK marked this pull request as ready for review July 3, 2021 23:57
@JoostK JoostK added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jul 3, 2021
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix @JoostK. I was a bit confused by the commit message, but I think the problem that is being solved is that in non-synthesized nodes, the TS compiler will shortcut having to render the node manually by just grabbing the text given by the "source-span". But in this case, since the node was moved from one file (the original source) to another (the type-check file) the parse-span points to a random segment of text in the type-check file. Is that correct?

// the type as a whole cannot be emitted in that case. Otherwise, the result of visiting all child
// nodes determines the result. If no ineligible type reference node is found then the walk
// returns `undefined`, indicating that no type node was visited that could not be emitted.
// If an unsupported type node is found at any position within the type and, then the `INELIGIBLE`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT:

Suggested change
// If an unsupported type node is found at any position within the type and, then the `INELIGIBLE`
// If an unsupported type node is found at any position within the type and then the `INELIGIBLE`

@JoostK
Copy link
Member Author

JoostK commented Jul 9, 2021

I was a bit confused by the commit message, but I think the problem that is being solved is that in non-synthesized nodes, the TS compiler will shortcut having to render the node manually by just grabbing the text given by the "source-span". But in this case, since the node was moved from one file (the original source) to another (the type-check file) the parse-span points to a random segment of text in the type-check file. Is that correct?

That's exactly the case, indeed!

@JoostK JoostK added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 9, 2021
…arameters in a different file

In angular#42492 the template type checker became capable of replicating a
wider range of generic type parameters for use in template type-check
files. Any literal types within a type parameter would however emit
invalid code, as TypeScript was emitting the literals using the text as
extracted from the template type-check file instead of the original
source file where the type node was taken from.

This commit works around the issue by cloning any literal types and
marking them as synthetic, signalling to TypeScript that the literal
text has to be extracted from the node itself instead from the source
file.

This commit also excludes `import()` type nodes from being supported,
as their module specifier may potentially need to be rewritten.

Fixes angular#42667
@JoostK JoostK force-pushed the ngtsc/ttc/type-emitter-literals branch from 81e0069 to 02fea47 Compare July 10, 2021 16:41
@JoostK JoostK added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jul 10, 2021
@AndrewKushnir
Copy link
Contributor

Presubmit.

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Jul 12, 2021
AndrewKushnir pushed a commit that referenced this pull request Jul 12, 2021
…arameters in a different file (#42761)

In #42492 the template type checker became capable of replicating a
wider range of generic type parameters for use in template type-check
files. Any literal types within a type parameter would however emit
invalid code, as TypeScript was emitting the literals using the text as
extracted from the template type-check file instead of the original
source file where the type node was taken from.

This commit works around the issue by cloning any literal types and
marking them as synthetic, signalling to TypeScript that the literal
text has to be extracted from the node itself instead from the source
file.

This commit also excludes `import()` type nodes from being supported,
as their module specifier may potentially need to be rewritten.

Fixes #42667

PR Close #42761
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler cla: yes P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incremental build - ngtypecheck - error TS1110: Type expected.
3 participants