-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
feat: valid-typeof
: always ban undefined
#15635
Conversation
I think we can evaluate that change separately from this PR / original issue. Just handling |
lib/rules/valid-typeof.js
Outdated
suggest: [ | ||
{ | ||
messageId: "suggestString", | ||
fix(fixer) { | ||
return fixer.replaceText(sibling, '"undefined"'); | ||
} | ||
} | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suggestion to replace undefined
with "undefined"
makes sense with requireStringLiterals: true
, too.
Maybe we could switch the order of else-if
s:
} else if (sibling.type === "Identifier" && sibling.name === "undefined" && isReferenceToGlobalVariable(sibling)) {
context.report({
node: sibling,
messageId: requireStringLiterals ? "notString" : "invalidValue",
suggest: [
{
messageId: "suggestString",
data: { type: "undefined" },
fix(fixer) {
return fixer.replaceText(sibling, '"undefined"');
}
}
]
});
} else if (requireStringLiterals && !isTypeofExpression(sibling)) {
context.report({ node: sibling, messageId: "notString" });
}
Test case:
{
code: "if (typeof bar !== undefined) {}",
options: [{ requireStringLiterals: true }],
errors: [
{
messageId: "notString",
type: "Identifier",
suggestions: [
{
messageId: "suggestString",
data: { type: "undefined" },
output: 'if (typeof bar !== "undefined") {}'
}
]
}
]
},
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just waiting on @mdjermanovic to confirm his feedback has been addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for contributing!
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.10.0` -> `8.11.0`](https://renovatebot.com/diffs/npm/eslint/8.10.0/8.11.0) | --- ### Release Notes <details> <summary>eslint/eslint</summary> ### [`v8.11.0`](https://github.com/eslint/eslint/releases/v8.11.0) [Compare Source](eslint/eslint@v8.10.0...v8.11.0) #### Features - [`800bd25`](eslint/eslint@800bd25) feat: add `destructuredArrayIgnorePattern` option in `no-unused-vars` ([#​15649](eslint/eslint#15649)) (Nitin Kumar) - [`8933fe7`](eslint/eslint@8933fe7) feat: Catch `undefined` and `Boolean()` in no-constant-condition ([#​15613](eslint/eslint#15613)) (Jordan Eldredge) - [`f90fd9d`](eslint/eslint@f90fd9d) feat: Add ESLint favicon to the HTML report document ([#​15671](eslint/eslint#15671)) (Mahdi Hosseinzadeh) - [`57b8a57`](eslint/eslint@57b8a57) feat: `valid-typeof` always ban `undefined` ([#​15635](eslint/eslint#15635)) (Zzzen) #### Bug Fixes - [`6814922`](eslint/eslint@6814922) fix: escaping for square brackets in ignore patterns ([#​15666](eslint/eslint#15666)) (Milos Djermanovic) - [`c178ce7`](eslint/eslint@c178ce7) fix: extend the autofix range in comma-dangle to ensure the last element ([#​15669](eslint/eslint#15669)) (Milos Djermanovic) #### Documentation - [`c481cec`](eslint/eslint@c481cec) docs: add fast-eslint-8 to atom integrations (userguide) ([#​15695](eslint/eslint#15695)) (db developer) - [`d2255db`](eslint/eslint@d2255db) docs: Add clarification about `eslint-enable` ([#​15680](eslint/eslint#15680)) (dosisod) - [`8b9433c`](eslint/eslint@8b9433c) docs: add object pattern to first section of computed-property-spacing ([#​15679](eslint/eslint#15679)) (Milos Djermanovic) - [`de800c3`](eslint/eslint@de800c3) docs: link to minimatch docs added. ([#​15688](eslint/eslint#15688)) (Gaurav Tewari) - [`8f675b1`](eslint/eslint@8f675b1) docs: sort-imports add single named import example ([#​15675](eslint/eslint#15675)) (Arye Eidelman) #### Chores - [`385c9ad`](eslint/eslint@385c9ad) chore: rm trailing space in docs ([#​15689](eslint/eslint#15689)) (唯然) </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1217 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
* feat: `valid-typeof`: always ban `undefined` * change error message and check definition of `undefined` * check string literal first * add suggestions * Apply suggestions from code review Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> * suggest string when `requireStringLiterals` is on Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
This reverts commit 8d9d0f7.
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
fixes #15591
What changes did you make? (Give an overview)
check
undefined
and reportIs there anything you'd like reviewers to focus on?
should we use
getStaticValue
?