8000 feat(compiler-cli): enable narrowing of signal reads in templates by JoostK · Pull Request #55456 · angular/angular · GitHub
[go: up one dir, main page]

Skip to content

feat(compiler-cli): enable narrowing of signal reads in templates #55456

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

Closed
wants to merge 1 commit into from

Conversation

JoostK
Copy link
Member
@JoostK JoostK commented Apr 21, 2024

Signal reads in Angular templates don't currently benefit from TypeScript's narrowing capabilities, since function calls are not eligle for type narrowing. This is unfortunate since Angular templates should not be updating signal values, such that narrowing would be a safe operation.

Ideally there would be a way for TypeScript to enable type-narrowing of signal reads, but this is unfortunately not possible today.

Various potential designs have been explored to support type-narrowing of signal reads in the compiler's Type-Check Block (TCB):

  1. Assign call expressions to a local variable in the TCB, rewriting further usages of the same signal to the local variable instead. This way, TypeScript is able to apply narrowing of this local variable that all subsequent reads are then able to leverage.

  2. Augment the Function declaration with a getter field corresponding with the function's return type, then emitting call expression as property reads using this augmented getter.

Although approach 2 simplifies the TCB emit compared to 1, it could not be made to work in practice as the Function declaration itself doesn't have any generic type to extract the return type from.

Instead, this commit implements approach 1. The primary known design limitation of this approach arrises with optional chaining, where the call expressions end up with different identities to prevent reusing an earlier variable declaration for the call expression. For example, consider the following template:

@if (person()?.address()) {
	<app-address [address]="person().address()">
}

Normally in TypeScript this would be accepted (provided that the call expressions are just property reads), but our referencing approach does not consider the signal reads of address() to have the same receiver. You may think this limitation can be overcome by always identifying property reads as equivalent to their safe counterpart (i.e. always treating . as ?.) but doing so would introduce false negatives in the negated case:

@if (!person()?.address()) {
	<app-address [address]="person().address()">
}

If the person().address() was to correspond with person()?.address() to make it align with the condition, then no error would be reported that person() may be null. To avoid such false negatives in template diagnostics this commit is being conservative with optional chains, where narrowing is not achieved as would be the case with property reads. Perhaps there may be ways of improving this in the future.

Another side effect of this change is that diagnostics won't be reported within expressions that have been substituted with a reference. Consider:

@if (person().nam()) {
	{{ person().nam() }}
}

Since the secondary occurrence of person().nam() is being substituted for the local variable that is assigned within the @if-condition, the typo in nam() is not reported for the expression in the block.


This is still a draft, the code needs more tests and we have to determine how to roll this out (i.e. opt-in vs opt-out vs always on).

@JoostK JoostK added area: compiler Issues related to `ngc`, Angular's template compiler target: minor This PR is targeted for the next minor release compiler: template type-checking cross-cutting: signals labels Apr 21, 2024
@ngbot ngbot bot added this to the Backlog milestone Apr 21, 2024
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Apr 21, 2024
@devversion devversion self-assigned this May 24, 2024
Signal reads in Angular templates don't currently benefit from TypeScript's
narrowing capabilities, since function calls are not eligible for type narrowing.
This is unfortunate since Angular templates should not be updating signal
values, such that narrowing would be a safe operation.

Ideally there would be a way for TypeScript to enable type-narrowing of signal
reads, but this is unfortunately not possible today.

Various potential designs have been explored to support type-narrowing of
signal reads in the compiler's Type-Check Block (TCB):

1. Assign call expressions to a local variable in the TCB, rewriting further
   usages of the same signal to the local variable instead. This way,
   TypeScript is able to apply narrowing of this local variable that all
   subsequent reads are then able to leverage.

2. Augment the `Function` declaration with a getter field corresponding with
   the function's return type, then emitting call expression as property reads
   using this augmented getter.

Although approach 2 simplifies the TCB emit compared to 1, it could not be made
to work in practice as the `Function` declaration itself doesn't have any
generic type to extract the return type from.

Instead, this commit implements approach 1. The primary known design limitation
of this approach arises with optional chaining, where the call expressions
end up with different identities to prevent reusing an earlier variable
declaration for the call expression. For example, consider the following template:

```
@if (person()?.address()) {
	<app-address [address]="person().address()">
}
```

Normally in TypeScript this would be accepted (provided that the call
expressions are just property reads), but our referencing approach does not
consider the signal reads of `address()` to have the same receiver. You may
think this limitation can be overcome by always identifying property reads
as equivalent to their safe counterpart (i.e. always treating `.` as `?.`) but
doing so would introduce false negatives in the negated case:

```
@if (!person()?.address()) {
	<app-address [address]="person().address()">
}
```

If the `person().address()` was to correspond with `person()?.address()` to
make it align with the condition, then no error would be reported that
`person()` may be `null`. To avoid such false negatives in template diagnostics
this commit is being conservative with optional chains, where narrowing is not
achieved as would be the case with property reads. Perhaps there may be ways of
improving this in the future.

Another side effect of this change is that diagnostics won't be reported within
expressions that have been substituted with a reference. Consider:

```
@if (person().nam()) {
	{{ person().nam() }}
}
```

Since the secondary occurrence of `person().nam()` is being substituted for the
local variable that is assigned within the `@if`-condition, the typo in `nam()`
is not reported for the expression in the block.
@crisbeto
Copy link
Member
crisbeto commented Jun 7, 2024

TGP results

@Harpush
Copy link
Harpush commented Sep 10, 2024

Is this still planned? Currently that's the only reason signals and control flow are still clunky

@JoostK
Copy link
Member Author
JoostK commented Sep 10, 2024

Is this still planned? Currently that's the only reason signals and control flow are still clunky

There's an issue with the current implementation that is yet to be resolved. It's not a dead end (yet) but no ETA right now.

@Harpush
Copy link
Harpush commented Sep 10, 2024

Is this still planned? Currently that's the only reason signals and control flow are still clunky

There's an issue with the current implementation that is yet to be resolved. It's not a dead end (yet) but no ETA right now.

What is the recommended workaround? I have a discriminated union signal I want to switch case on its type. I thought @let will allow it but I must choose a new name. So what should one do in those cases for now?

@JoostK
Copy link
Member Author
JoostK commented Sep 10, 2024

Use @let. If #55183 lands you can disambiguate the signal read from the template variable using @let name = this.name(), until then a different name is required.

@JeanMeche
Copy link
Member

Doing some cleanup, I'll allow myself to close the PR since we're not going in this direction.

@JeanMeche JeanMeche closed this Feb 27, 2025
@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 Mar 30, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: compiler Issues related to `ngc`, Angular's template compiler compiler: template type-checking cross-cutting: signals detected: feature PR contains a feature commit target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0