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

Support Number.NaN within use-isnan #14715

Closed
esdmr opened this issue Jun 16, 2021 · 3 comments · Fixed by #14718
Closed

Support Number.NaN within use-isnan #14715

esdmr opened this issue Jun 16, 2021 · 3 comments · Fixed by #14718
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly repro:yes rule Relates to ESLint's core rules
Projects

Comments

@esdmr
Copy link

esdmr commented Jun 16, 2021

What rule do you want to change?
use-isnan

Does this change cause the rule to produce more or fewer warnings?
More.

How will the change be implemented? (New option, new default behavior, etc.)?
Perhaps as a default behaviour, as NaN and Number.NaN are equivalent.

Please provide some example code that this change will affect:

x === NaN;
x === Number.NaN;

What does the rule currently do for this code?
Only the first line is reported as a NaN comparison. Second line does not report.

What will the rule do after it's changed?
Both lines will report as NaN comparisons. (Maybe recommending Number.isNaN instead of isNaN but that is aside)

Are you willing to submit a pull request to implement this change?
I would prefer if someone else send a PR.

At lib/rules/use-isnan.js:24:

-    return Boolean(node) && node.type === "Identifier" && node.name === "NaN";
+    return Boolean(node) && (node.type === "Identifier" && node.name === "NaN" ||
+        node.type === "MemberExpression" &&
+        node.object.type === "Identifier" && node.object.name === "Number" &&
+        node.property.type === "Identifier" && node.property.name === "NaN");
@esdmr esdmr added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon labels Jun 16, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Jun 16, 2021
@snitin315
Copy link
Contributor

snitin315 commented Jun 17, 2021

Looks like a bug to me. Online Demo

I would prefer if someone else send a PR.

I can send a PR if this issue is accepted.

@snitin315 snitin315 added bug ESLint is working incorrectly repro:yes and removed enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon labels Jun 17, 2021
@aladdin-add
Copy link
Member

agreed it was a bug.

@aladdin-add aladdin-add moved this from Needs Triage to Ready to Implement in Triage Jun 17, 2021
@nzakas nzakas added the accepted There is consensus among the team that this change meets the criteria for inclusion label Jun 17, 2021
@nzakas
Copy link
Member

nzakas commented Jun 17, 2021

Agree, looks like a bug.

@mdjermanovic mdjermanovic moved this from Ready to Implement to Pull Request Opened in Triage Jun 26, 2021
Triage automation moved this from Pull Request Opened to Complete Jun 26, 2021
mdjermanovic added a commit that referenced this issue Jun 26, 2021
…14718)

* Update: improve `isNaNIdentifier` to detect `Number.isNaN` (fixes #14715)

* Chore: add test cases for `Number.NaN`

* Docs: add more examples for `use-isnan`

* Chore: improve logic and add more test cases

* Docs: Update docs/rules/use-isnan.md

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 24, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly repro:yes rule Relates to ESLint's core rules
Projects
Archived in project
Triage
Complete
Development

Successfully merging a pull request may close this issue.

4 participants