-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Added no-unsafe
rule
#1831
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
Added no-unsafe
rule
#1831
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.
This rule should only be active when the React pragma version is in a range that supports the unsafe methods - 16.3 or 16.4+, i think?
lib/rules/no-unsafe.js
Outdated
|
||
context.report({ | ||
node: node, | ||
message: `Do not use ${method}` |
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 message should explain why, and link to more info
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.
Added details
tests/lib/rules/no-unsafe.js
Outdated
`, | ||
errors: [ | ||
{ | ||
message: 'Do not use UNSAFE_componentWillMount' |
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.
these tests should include position information, so that we can test that part as well
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.
Added a few more tests and position details for invalid cases
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.
It seems that added position details causes issues in eslint 3, see TEST=true ESLINT=3
failed travis builds.
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.
Adjusted tests to support ESLint 3
The rule is applicable for 16.3.0+ only now, good point. |
lib/rules/no-unsafe.js
Outdated
@@ -44,7 +45,8 @@ module.exports = { | |||
*/ | |||
function isUnsafe(method) { | |||
const unsafeMethods = getUnsafeMethods(); | |||
return unsafeMethods.indexOf(method) !== -1; | |||
const isApplicable = versionUtil.testReactVersion(context, '16.3.0'); |
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.
This seems like a check we could do earlier, and prevent even returning any visitors in the first place?
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.
Added early termination for the rule.
index.js
Outdated
@@ -139,6 +140,7 @@ module.exports = { | |||
'react/no-string-refs': 2, | |||
'react/no-unescaped-entities': 2, | |||
'react/no-unknown-property': 2, | |||
'react/no-unsafe': 2, |
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.
enabling new rules in an exported config is a semver-major change; please set this to 0 for now.
lib/rules/no-unsafe.js
Outdated
docs: { | ||
description: 'Prevent usage of UNSAFE_ methods', | ||
category: 'Best Practices', | ||
recommended: true, |
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.
ditto
Excluded the rule from the recommended for now. Would we like to track it somehow to turn on it in the next major release? |
Why not build this logic into the existing no-deprecated rule as opposed to creating a new dedicated rule? |
I don't consider them deprecated yet, because there's not a replacement for all of their use cases. |
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
Added
no-unsafe
rule to address the corresponding issue #1830. See discussion details #1750 (comment).Fixes #1830