feat(eslint-plugin): add no-uncalled-signals rule #2383
Conversation
fc8bd24 to
41eaa68
Compare
There was a problem hiding this comment.
TypeScript has always referred to this as uncalled function checks: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#uncalled-function-checks
So maybe no-uncalled-signals is a better name (given it doesn't check any other kind of "misuse")?
|
WDYT about the name @reduckted? |
|
I think |
6826326 to
74db158
Compare
|
Thanks for the feedback! I've reverted the |
74db158 to
fd51cce
Compare
|
View your CI Pipeline Execution ↗ for commit 3cae292.
☁️ Nx Cloud last updated this comment at |
|
@scj7t4 Thanks please see the CI failures they tell you what to do to fix them |
| description: | ||
| "Warns user about unintentionally doing logic on the signal, rather than the signal's value", | ||
| }, | ||
| hasSuggestions: true, |
There was a problem hiding this comment.
The rule is marked with hasSuggestions: true but doesn't implement any suggestions in the context.report() call. To properly support suggestions, either:
-
Add a
suggestproperty to the report object with appropriate fixes:context.report({ node, messageId: 'noUncalledSignals', suggest: [ { messageId: 'addCallParentheses', fix: (fixer) => fixer.replaceText(node, `${node.name}()`) } ] });
-
Or remove the
hasSuggestions: trueproperty if no suggestions will be provided.
This would make the rule's behavior consistent with its metadata.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Checks that signals used in logical expressions are invoked. For angular-eslint#2302
f2adafb to
33e6dfc
Compare
Should be fixed |
|
@scj7t4 You'll need to run these two commands to update some generated files: |
| export type MessageIds = 'noUncalledSignals'; | ||
| export const RULE_NAME = 'no-uncalled-signals'; | ||
|
|
||
| const KNOWN_SIGNAL_TYPES: ReadonlySet<string> = new Set([ |
There was a problem hiding this comment.
This is duplicated from the prefer-signals rule. You might want to move it into a common place (maybe ./utils/signals.ts) so that both rules can share it.
| ESLintUtils.getParserServices(context); | ||
|
|
||
| return { | ||
| '*.test[type=Identifier], *.test Identifier'(node: TSESTree.Identifier) { |
There was a problem hiding this comment.
Nice use of *.test. 👍
There are some cases that it doesn't find though:
const v = aSignal || true;const v = aSignal ?? true;There was a problem hiding this comment.
I'm struggling a bit with this one. If I add the matcher:
[type=LogicalExpression] Identifier
I can find these cases. However, then I can't find a variation of the first matcher (or my new matcher) that will not match for both in some cases. Is it okay to have a rule report the same finding 2 times?
I've pushed the new version with the extra matcher.
| const identifierType = type.getSymbol()?.name; | ||
|
|
||
| if (identifierType && KNOWN_SIGNAL_TYPES.has(identifierType)) { | ||
| context.report({ |
There was a problem hiding this comment.
Since the most likely change to fix this problem will be to call the signal, it might be worth including a suggestion that adds () after the identifier. It won't auto-fix it, but it will provide a quick way for users to apply the code change to get rid of this problem.
There was a problem hiding this comment.
Agreed, it's the exact thing graphite suggested above #2383 (comment)
There was a problem hiding this comment.
Added a suggestion and adjusted the tests to verify the suggestion.
…logical expression test case
Checks that signals used in logical expressions are invoked. For #2302
Thanks for your work on the project! This is my first attempt at a typescript eslint plugin, so feedback is very appreciated on what I messed up :)