-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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: update regex for methods with thisArg
#17439
Conversation
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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 change overall looks good to me. I left one suggestion.
I believe this should be "feat" because we are adding support for more recent methods.
lib/rules/utils/ast-utils.js
Outdated
const bindOrCallOrApplyPattern = /^(?:bind|call|apply)$/u; | ||
const methodWithThisArgPattern = /^(?:every|filter|find(?:Last)?(?:Index)?|flatMap|forEach|map|some)$/u; |
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.
Should we keep "array" in the name?
const methodWithThisArgPattern = /^(?:every|filter|find(?:Last)?(?:Index)?|flatMap|forEach|map|some)$/u; | |
const arrayMethodWithThisArgPattern = /^(?:every|filter|find(?:Last)?(?:Index)?|flatMap|forEach|map|some)$/u; |
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.
I think it's fine to keep array
since these are all names of array methods at this time. We will notice if the situation will change one day. Updated in 01dfe5a, thanks.
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! Leaving open for @nzakas to verify.
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
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:
What rule do you want to change?
no-eval
andno-invalid-this
What change do you want to make (place an "X" next to just one item)?
[ ] Generate more warnings
[X] Generate fewer warnings
[ ] Implement autofix
[ ] Implement suggestions
How will the change be implemented (place an "X" next to just one item)?
[ ] A new option
[X] A new default behavior
[ ] Other
Please provide some example code that this change will affect:
Playground Link
Find more examples in the updated unit tests.
What does the rule currently do for this code?
Report an error for each rule.
What will the rule do after it's changed?
Report no error.
What changes did you make? (Give an overview)
This PR updates a shared regex in
ast-utils.js
to add support for method names that were added to the spec since ES2019.The regex is used in the rules
no-eval
andno-invalid-this
to avoid flagging some special cases, trivially relying on the name of a method being called.So the net effect of this PR will be less problems reported by those two rules.
The updated regex is intended to match names of built-in methods that expect a callback function as a first argument, and the value bound to
this
in calls to the callback as a second argument.These are mostly methods of arrays and typed arrays, although
forEach
is also defined on sets and maps.The new method names in this PR are:
findLast
(added in ES2023 toArray.prototype
andTypedArray.prototype
)findLastIndex
(added in ES2023 toArray.prototype
andTypedArray.prototype
)flatMap
(added in ES2019 toArray.prototype
)I've also renamed the regex for more clarity and added unit tests for the
no-eval
rule and extended the tests forno-invalid-this
.Is there anything you'd like reviewers to focus on?
fix
orfeat
?astUtils.isDefaultThisBinding
is currently only tested indirectly. Do we need any specific unit tests?