fix(compiler-cli): catch function instance properties in interpolated signal diagnostic #54325
Conversation
jessicajaniuk
added
the
area: compiler
| // check for `{{ mySignal.name }}` or `{{ mySignal.length }}` or `{{ mySignal.prototype }}` | ||
| // as these are the name of instance properties of Function, the compiler does _not_ throw an error | ||
| const symbolOfReceiver = ctx.templateTypeChecker.getSymbolOfNode(node.receiver, component); | ||
| if (isFunctionInstanceProperty(node.name) && symbolOfReceiver?.kind === SymbolKind.Expression && (isSignal(symbolOfReceiver.tsType.symbol) || isSignal(symbolOfReceiver.tsType.aliasSymbol))) { |
There was a problem hiding this comment.
Would it make sense to always report a diagnostic if the receiver is a signal and not invoked? e.g.
{{mySignal.set()}}should not be supported anyway- `x="{{mySignal.set()}} neither
- etc.
or do I miss something? Just a high-level comment from a quick glance
There was a problem hiding this comment.
Most cases should already be caught by the compiler (for example {{ mySignal.version }}) but I suppose {{ mySignal.set }} or {{ mySignal.update }} aren't and we probably should. It may never make sense to have a signal receiver that is not invoked, you're right.
Let me know if the team thinks we should report this, and I'll update the PR.
There was a problem hiding this comment.
How is .version caught btw? In either case, I think we can forbid set and .update as well. I'd propose doing that change
There was a problem hiding this comment.
Something like version is caught because it does not exist on WritableSignal so the compiler throws.
I updated the PR to catch set, update, and asReadonly 👍
There was a problem hiding this comment.
I wonder if asReadonly might not be useful to allow, in property bindings, but we can always change
cexbrayat
force-pushed
the
fix/function-property-in-signal-interpolation
branch
5 times, most recently
from
57fa4aa
to
f3e6910
Compare
devversion
added
action: merge
|
@cexbrayat can you please rebase? |
cexbrayat
force-pushed
the
fix/function-property-in-signal-interpolation
branch
from
f3e6910
to
19f6fbd
Compare
… signal diagnostic
Currently, the following template compiles without error, even if the signal is not properly called:
```
<div>{{ mySignal.name }}</div>
```
This is because `name` is one of the instance properties of Function (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function#instance_properties).
The interpolated signal diagnostic is now extended to catch such issues.
cexbrayat
force-pushed
the
fix/function-property-in-signal-interpolation
branch
from
19f6fbd
to
4ed94ba
Compare
|
@devversion Done |
|
This PR was merged into the repository by commit f578889. |
… signal diagnostic (#54325) Currently, the following template compiles without error, even if the signal is not properly called: ``` <div>{{ mySignal.name }}</div> ``` This is because `name` is one of the instance properties of Function (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function#instance_properties). The interpolated signal diagnostic is now extended to catch such issues. PR Close #54325
|
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. |
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?
Currently, the following template compiles without error, even if the signal is not properly called:
This is because
nameis one of the instance properties of Function (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function#instance_properties).What is the new behavior?
The interpolated signal diagnostic is now extended to catch such issues.
Does this PR introduce a breaking change?
Other information