-
-
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
Update: Handle locally unsupported regex in computed property keys #12056
Conversation
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!
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!
Sorry, to avoid confusion I usually wait to review until after a change is accepted. Code LGTM and I support this change, but we’ll need another 👍🏼 And a champion. |
Would you try getPropertyName function of |
Sorry, I should be more specific and the title/commit message indeed sounds like this is an enhancement. This was supposed to be a bug fix. The title has 'Update' prefix because the fix can produce more warnings. The function already supports regexp literals, but it doesn't 'support' RegExpLiteral extension of the Literal interface, i.e. the The bug is in |
I'll try it on Node 8 with test cases from this PR to be sure, but I think it doesn't use Literal(node) {
//istanbul ignore if : this is implementation-specific behavior.
if (node.regex != null && node.value == null) {
// It was a RegExp literal, but Node.js didn't support it.
return null
}
return node
}, instead of return { value: `/${node.regex.pattern}/${node.regex.flags}` } or maybe there is a specific reason to return
Looks like a lot more, even without the scope argument, I'm not sure would it be a breaking change? |
I think I see the reason now, |
Changed the title and commit message. I still believe this is a bug fix rather than enhancement :) |
@mysticatea Have your concerns been addressed? |
I apologize for I lost track on this. LGTM. |
@mysticatea no problem, but this will now collide with #12701, which handles this + bigints as well. Might be easier to close this PR and do everything in #12701? |
Rebased and fixed conflicts & JSDoc, for the case that this gets merged. |
Thank you! |
What is the purpose of this pull request? (put an "X" next to item)
[X] Bug fix
Tell us about your environment
What parser (default, Babel-ESLint, etc.) are you using?
default
Please show your full configuration:
Configuration
What did you do? Please include the actual source code causing the issue.
This bug is observable in environments that don't support ES2018 regexp syntax, namely Node 8.
For the online demo use Firefox (current Firefox version is 68, still doesn't support this syntax), results are different when compared to Chrome.
This bug affects 5 rules:
no-dupe-keys
false negative:Demo link (open in Firefox)
no-dupe-keys
false positive:Demo link (open in Firefox)
sort-keys
false negative:Demo link (open in Firefox)
sort-keys
false positive:Demo link (open in Firefox)
no-self-assign
false negative:Demo link (open in Firefox)
no-self-assign
false positive:Demo link (open in Firefox)
yoda
false negative:Demo link (open in Firefox)
yoda
false positive:Demo link (open in Firefox)
no-restricted-properties
false negative:Demo link (open in Firefox)
no-restricted-properties
false positive:Demo link (open in Firefox)
What did you expect to happen?
Warnings for 'false negative' examples, no warnings for 'false positive' examples.
What actually happened? Please include the actual, raw output from ESLint.
The opposite.
What changes did you make? (Give an overview)
Fixed
astUtils.getStaticPropertyName
Is there anything you'd like reviewers to focus on?
Part of the code is extracted to new function
getStaticStringValue
, it's easier to test and it might be useful for something else.Unrelated to this PR:
accesor-pairs
should usegetStaticPropertyName
, but this rule doesn't work well. It should enforce pairs per key, not per the whole literal. I'm working on this, a PR will be ready very soon.getStaticPropertyName
instead of a partial/duplicate logic, I'm working on this.