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

Added no-unsafe rule #1831

Merged
merged 5 commits into from Jun 21, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
Expand Up @@ -117,6 +117,7 @@ Enable the rules that you would like to use.
* [react/no-this-in-sfc](docs/rules/no-this-in-sfc.md): Prevent using `this` in stateless functional components
* [react/no-unescaped-entities](docs/rules/no-unescaped-entities.md): Prevent invalid characters from appearing in markup
* [react/no-unknown-property](docs/rules/no-unknown-property.md): Prevent usage of unknown DOM property (fixable)
* [react/no-unsafe](docs/rules/no-unsafe.md): Prevent usage of `UNSAFE_` methods
* [react/no-unused-prop-types](docs/rules/no-unused-prop-types.md): Prevent definitions of unused prop types
* [react/no-unused-state](docs/rules/no-unused-state.md): Prevent definitions of unused state properties
* [react/no-will-update-set-state](docs/rules/no-will-update-set-state.md): Prevent usage of `setState` in `componentWillUpdate`
Expand Down Expand Up @@ -208,6 +209,7 @@ The rules enabled in this configuration are:
* [react/no-string-refs](docs/rules/no-string-refs.md)
* [react/no-unescaped-entities](docs/rules/no-unescaped-entities.md)
* [react/no-unknown-property](docs/rules/no-unknown-property.md)
* [react/no-unsafe](docs/rules/no-unsafe.md)
* [react/prop-types](docs/rules/prop-types.md)
* [react/react-in-jsx-scope](docs/rules/react-in-jsx-scope.md)
* [react/require-render-return](docs/rules/require-render-return.md)
Expand Down
46 changes: 46 additions & 0 deletions docs/rules/no-unsafe.md
@@ -0,0 +1,46 @@
# Prevent usage of `UNSAFE_` methods (react/no-unsafe)

Certain legacy lifecycle methods are unsafe for use in async React applications and cause warnings in [_strict mode_][strict_mode]. These also happen to be the lifecycles that cause the most [confusion within the React community][component_lifecycle_changes].

[strict_mode]: https://reactjs.org/docs/strict-mode.html#identifying-unsafe-lifecycles
[component_lifecycle_changes]: https://reactjs.org/blog/2018/03/29/react-v-16-3.html#component-lifecycle-changes

The rule checks the following methods: `UNSAFE_componentWillMount`, `UNSAFE_componentWillReceiveProps`, `UNSAFE_componentWillUpdate`.

## Rule Details

The following patterns are considered warnings:

```jsx
class Foo extends React.Component {
UNSAFE_componentWillMount() {}
UNSAFE_componentWillReceiveProps() {}
UNSAFE_componentWillUpdate() {}
}
```

```jsx
const Foo = createReactClass({
UNSAFE_componentWillMount: function() {},
UNSAFE_componentWillReceiveProps: function() {},
UNSAFE_componentWillUpdate: function() {}
});
```

The following patterns are **not** considered warnings:

```jsx
class Foo extends Bar {
UNSAFE_componentWillMount() {}
UNSAFE_componentWillReceiveProps() {}
UNSAFE_componentWillUpdate() {}
}
```

```jsx
const Foo = bar({
UNSAFE_componentWillMount: function() {},
UNSAFE_componentWillReceiveProps: function() {},
UNSAFE_componentWillUpdate: function() {}
});
```
2 changes: 2 additions & 0 deletions index.js
Expand Up @@ -64,6 +64,7 @@ const allRules = {
'no-typos': require('./lib/rules/no-typos'),
'no-unescaped-entities': require('./lib/rules/no-unescaped-entities'),
'no-unknown-property': require('./lib/rules/no-unknown-property'),
'no-unsafe': require('./lib/rules/no-unsafe'),
'no-unused-prop-types': require('./lib/rules/no-unused-prop-types'),
'no-unused-state': require('./lib/rules/no-unused-state'),
'no-will-update-set-state': require('./lib/rules/no-will-update-set-state'),
Expand Down Expand Up @@ -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,
Copy link
Member

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.

'react/prop-types': 2,
'react/react-in-jsx-scope': 2,
'react/require-render-return': 2
Expand Down
93 changes: 93 additions & 0 deletions lib/rules/no-unsafe.js
@@ -0,0 +1,93 @@
/**
* @fileoverview Prevent usage of UNSAFE_ methods
* @author Sergei Startsev
*/

'use strict';

const Components = require('../util/Components');
const astUtil = require('../util/ast');
const docsUrl = require('../util/docsUrl');

// ------------------------------------------------------------------------------
// Rule Definition
// ------------------------------------------------------------------------------

module.exports = {
meta: {
docs: {
description: 'Prevent usage of UNSAFE_ methods',
category: 'Best Practices',
recommended: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

url: docsUrl('no-unsafe')
},
schema: []
},

create: Components.detect((context, components, utils) => {
/**
* Returns a list of unsafe methods
* @returns {Array} A list of unsafe methods
*/
function getUnsafeMethods() {
return [
'UNSAFE_componentWillMount',
'UNSAFE_componentWillReceiveProps',
'UNSAFE_componentWillUpdate'
];
}

/**
* Checks if a passed method is unsafe
* @param {string} method Life cycle method
* @returns {boolean} Returns true for unsafe methods, otherwise returns false
*/
function isUnsafe(method) {
const unsafeMethods = getUnsafeMethods();
return unsafeMethods.indexOf(method) !== -1;
}

/**
* Reports the error for an unsafe method
* @param {ASTNode} node The AST node being checked
* @param {string} method Life cycle method
*/
function checkUnsafe(node, method) {
if (!isUnsafe(method)) {
return;
}

context.report({
node: node,
message: `Do not use ${method}`
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added details

});
}

/**
* Returns life cycle methods if available
* @param {ASTNode} node The AST node being checked.
* @returns {Array} The array of methods.
*/
function getLifeCycleMethods(node) {
const properties = astUtil.getComponentProperties(node);
return properties.map(property => astUtil.getPropertyName(property));
}

/**
* Checks life cycle methods
* @param {ASTNode} node The AST node being checked.
*/
function checkLifeCycleMethods(node) {
if (utils.isES5Component(node) || utils.isES6Component(node)) {
const methods = getLifeCycleMethods(node);
methods.forEach(method => checkUnsafe(node, method));
}
}

return {
ClassDeclaration: checkLifeCycleMethods,
ClassExpression: checkLifeCycleMethods,
ObjectExpression: checkLifeCycleMethods
};
})
};
107 changes: 107 additions & 0 deletions tests/lib/rules/no-unsafe.js
@@ -0,0 +1,107 @@
/**
* @fileoverview Prevent usage of UNSAFE_ methods
* @author Sergei Startsev
*/
'use strict';

// ------------------------------------------------------------------------------
// Requirements
// ------------------------------------------------------------------------------

const rule = require('../../../lib/rules/no-unsafe');
const RuleTester = require('eslint').RuleTester;

const parserOptions = {
ecmaVersion: 2018,
sourceType: 'module',
ecmaFeatures: {
jsx: true
}
};

// ------------------------------------------------------------------------------
// Tests
// ------------------------------------------------------------------------------

const ruleTester = new RuleTester({parserOptions});
ruleTester.run('no-unsafe', rule, {
valid: [
{
code: `
class Foo extends React.Component {
componentDidUpdate() {}
render() {}
}
`
},
{
code: `
const Foo = createReactClass({
componentDidUpdate: function() {},
render: function() {}
});
`
},
{
code: `
class Foo extends Bar {
UNSAFE_componentWillMount() {}
UNSAFE_componentWillReceiveProps() {}
UNSAFE_componentWillUpdate() {}
}
`
},
{
code: `
const Foo = bar({
UNSAFE_componentWillMount: function() {},
UNSAFE_componentWillReceiveProps: function() {},
UNSAFE_componentWillUpdate: function() {},
});
`
}
],

invalid: [
{
code: `
class Foo extends React.Component {
UNSAFE_componentWillMount() {}
UNSAFE_componentWillReceiveProps() {}
UNSAFE_componentWillUpdate() {}
}
`,
errors: [
{
message: 'Do not use UNSAFE_componentWillMount'
Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

},
{
message: 'Do not use UNSAFE_componentWillReceiveProps'
},
{
message: 'Do not use UNSAFE_componentWillUpdate'
}
]
},
{
code: `
const Foo = createReactClass({
UNSAFE_componentWillMount: function() {},
UNSAFE_componentWillReceiveProps: function() {},
UNSAFE_componentWillUpdate: function() {},
});
`,
errors: [
{
message: 'Do not use UNSAFE_componentWillMount'
},
{
message: 'Do not use UNSAFE_componentWillReceiveProps'
},
{
message: 'Do not use UNSAFE_componentWillUpdate'
}
]
}
]
});