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

[no-signal-compare?] new rule request to prevent comparing signals via < or > #1753

Open
skyleguy opened this issue Mar 24, 2024 · 1 comment
Labels
package: eslint-plugin Angular-specific TypeScript rules triage This issue needs to be looked at and categorized by a maintainer

Comments

@skyleguy
Copy link

skyleguy commented Mar 24, 2024

I would like to propose a new rule getting added to angular-eslint that will prevent two signals from being compared via <, <=, >, >= as it does not really make sense. After attending NgConf I noticed that it is very easy for even an expert angular developer to accidentally forgot to unbox two signals when trying to compare their unboxed values like so:

sig1 = signal(5)
sig2 = signal(2)

if (sig1 > sig2) // will never be true
if (sig1() > sig2()) // will be true in this case and is most likely always what we'd be going for in this scenario

it does seem valid to compare signals with === so it does seem like that should be allowed.

I did open this request to @angular/core and was suggested that I open the feature request here instead. Here is the closed issue from @angular/core

it seems it would be very helpful to get some warning/error feedback when attempts to do this are made and could possibly even have a quickFix option that just adds the calling of the signal instead of using the signal itself as experienced and new devs alike could spend plenty of time trying to figure out why their conditions are not being hit correctly when trying to use signals. The text could be similar to what we see when trying to compare numbers and strings?

image

@skyleguy skyleguy added package: eslint-plugin Angular-specific TypeScript rules triage This issue needs to be looked at and categorized by a maintainer labels Mar 24, 2024
@KevinKelchen
Copy link

KevinKelchen commented Apr 18, 2024

I wonder if this could be expanded beyond comparison operators and apply to expressions in general. Expressions that evaluate to truthy/falsy would be common scenarios, but the scope would probably go beyond that.

A couple of examples:

This would flag as an issue:

if (this.mySignal)

This would also flag as an issue:

const myComputed = computed(() => this.mySignal + 1);

The rule could be disabled like other lint rules such as for the line that follows or whole sections of code. This could be an opt-out for the unlikely scenarios where we intend to check the signal reference itself instead of its unwrapped value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: eslint-plugin Angular-specific TypeScript rules triage This issue needs to be looked at and categorized by a maintainer
Projects
None yet
Development

No branches or pull requests

2 participants