Skip to content

Adjusted no-deprecated rule for React 16.3.0 #1750

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 10, 2018
Merged

Adjusted no-deprecated rule for React 16.3.0 #1750

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5ab7a23
Adjusted no-deprecated rule for React 16.3.0
sergei-startsev Mar 29, 2018
501edbf
Added `refs` field to refer to docs in`no-deprecated` rule, minor adj…
sergei-startsev Apr 1, 2018
7034ffc
Changed warnings in `no-deprecated` rule, deprecated old lifecycle me…
sergei-startsev Apr 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/rules/no-deprecated.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,17 +79,17 @@ module.exports = {
// 16.3.0
deprecated.componentWillMount = [
'16.3.0',
'constructor',
'UNSAFE_componentWillMount',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillmount'
];
deprecated.componentWillReceiveProps = [
'16.3.0',
'getDerivedStateFromProps',
'UNSAFE_componentWillReceiveProps',
Copy link

Choose a reason for hiding this comment

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

Why not suggest getDerivedStateFromProps? And why not lint against UNSAFE_ lifecycle methods?

Copy link
Member

Choose a reason for hiding this comment

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

Because that's only one of the use cases for componentWillReceiveProps, so it's not always the right recommendation.

Copy link

Choose a reason for hiding this comment

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

What's the point of linting against componentWillReceiveProps if we're just going to recommend UNSAFE_componentWillReceiveProps anyway? Both are equally bad, no? They do the exact same thing, just one has a scary name.

Copy link
Member

Choose a reason for hiding this comment

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

So that a human can look at it and make a human judgement about which to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sahrens, legacy lifecycles (including componentWillReceiveProps) will continue to work until version 17, however you will see deprecation warnings for them. In version 17, it will still be possible to use them, but they will be aliased with an “UNSAFE_”, see details.

Copy link
Member

Choose a reason for hiding this comment

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

I think that no-deprecated can and will warn against them, but not until every use case of them has a replacement (this is not currently the case).

Copy link
Contributor Author

@sergei-startsev sergei-startsev Jun 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

There're no deprecation warnings regarding using UNSAFE_ methods in strict mode, there is unsafe lifecycle methods warning:
unsafe lifecycle methods warning
So I'm not sure that no-deprecated is appropriate place for that (at least for now).
I'm also not sure regarding strict-mode rule, I vote for adding no-unsafe rule that will be more specific. In the same way that it was done for no-string-refs rule -- using string ref API also cause warnings in strict mode. @ljharb, I could take a look at it if you find it reasonable.

Copy link
Member

Choose a reason for hiding this comment

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

Modes are not a conceptual pattern I'd want to encourage, "strict" or otherwise.

I don't think a "no-unsafe" rule makes sense when for users of React < 16, it is perfectly safe, and for users of React >= 17, the methods won't work at all.

The purpose of these deprecations from React is to encourage people to migrate to replacements as early as possible, so the breaking change in v17 that removes the old ones will be as painless as possible. When React 17 comes out, we'll certainly lint against invalid lifecycle methods, including newly-invalid ones - but I'm not sure how much value we'd get out of warnings in React 16 outside of no-deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think a "no-unsafe" rule makes sense when for users of React < 16, it is perfectly safe, and for users of React >= 17, the methods won't work at all.

It's not true, even in version 17, it will still be possible to use UNSAFE_ methods, see Component Lifecycle Changes.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, a separate "no-unsafe" rule, that only focuses on UNSAFE_ methods, seems reasonable.

'https://reactjs.org/docs/react-component.html#unsafe_componentwillreceiveprops'
];
deprecated.componentWillUpdate = [
'16.3.0',
'getDerivedStateFromProps',
'UNSAFE_componentWillUpdate',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillupdate'
];
return deprecated;
Expand Down
36 changes: 18 additions & 18 deletions tests/lib/rules/no-deprecated.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,18 +221,18 @@ ruleTester.run('no-deprecated', rule, {
errors: [
{
message: errorMessage(
'componentWillMount', '16.3.0', 'constructor',
'componentWillMount', '16.3.0', 'UNSAFE_componentWillMount',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillmount'
)
},
{
message: errorMessage(
'componentWillReceiveProps', '16.3.0', 'getDerivedStateFromProps',
'componentWillReceiveProps', '16.3.0', 'UNSAFE_componentWillReceiveProps',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillreceiveprops'
)
},
{
message: errorMessage('componentWillUpdate', '16.3.0', 'getDerivedStateFromProps',
message: errorMessage('componentWillUpdate', '16.3.0', 'UNSAFE_componentWillUpdate',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillupdate'
)
}
Expand All @@ -251,18 +251,18 @@ ruleTester.run('no-deprecated', rule, {
errors: [
{
message: errorMessage(
'componentWillMount', '16.3.0', 'constructor',
'componentWillMount', '16.3.0', 'UNSAFE_componentWillMount',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillmount'
)
},
{
message: errorMessage(
'componentWillReceiveProps', '16.3.0', 'getDerivedStateFromProps',
'componentWillReceiveProps', '16.3.0', 'UNSAFE_componentWillReceiveProps',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillreceiveprops'
)
},
{
message: errorMessage('componentWillUpdate', '16.3.0', 'getDerivedStateFromProps',
message: errorMessage('componentWillUpdate', '16.3.0', 'UNSAFE_componentWillUpdate',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillupdate'
)
}
Expand All @@ -279,18 +279,18 @@ ruleTester.run('no-deprecated', rule, {
errors: [
{
message: errorMessage(
'componentWillMount', '16.3.0', 'constructor',
'componentWillMount', '16.3.0', 'UNSAFE_componentWillMount',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillmount'
)
},
{
message: errorMessage(
'componentWillReceiveProps', '16.3.0', 'getDerivedStateFromProps',
'componentWillReceiveProps', '16.3.0', 'UNSAFE_componentWillReceiveProps',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillreceiveprops'
)
},
{
message: errorMessage('componentWillUpdate', '16.3.0', 'getDerivedStateFromProps',
message: errorMessage('componentWillUpdate', '16.3.0', 'UNSAFE_componentWillUpdate',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillupdate'
)
}
Expand All @@ -307,18 +307,18 @@ ruleTester.run('no-deprecated', rule, {
errors: [
{
message: errorMessage(
'componentWillMount', '16.3.0', 'constructor',
'componentWillMount', '16.3.0', 'UNSAFE_componentWillMount',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillmount'
)
},
{
message: errorMessage(
'componentWillReceiveProps', '16.3.0', 'getDerivedStateFromProps',
'componentWillReceiveProps', '16.3.0', 'UNSAFE_componentWillReceiveProps',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillreceiveprops'
)
},
{
message: errorMessage('componentWillUpdate', '16.3.0', 'getDerivedStateFromProps',
message: errorMessage('componentWillUpdate', '16.3.0', 'UNSAFE_componentWillUpdate',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillupdate'
)
}
Expand All @@ -335,18 +335,18 @@ ruleTester.run('no-deprecated', rule, {
errors: [
{
message: errorMessage(
'componentWillMount', '16.3.0', 'constructor',
'componentWillMount', '16.3.0', 'UNSAFE_componentWillMount',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillmount'
)
},
{
message: errorMessage(
'componentWillReceiveProps', '16.3.0', 'getDerivedStateFromProps',
'componentWillReceiveProps', '16.3.0', 'UNSAFE_componentWillReceiveProps',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillreceiveprops'
)
},
{
message: errorMessage('componentWillUpdate', '16.3.0', 'getDerivedStateFromProps',
message: errorMessage('componentWillUpdate', '16.3.0', 'UNSAFE_componentWillUpdate',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillupdate'
)
}
Expand All @@ -363,18 +363,18 @@ ruleTester.run('no-deprecated', rule, {
errors: [
{
message: errorMessage(
'componentWillMount', '16.3.0', 'constructor',
'componentWillMount', '16.3.0', 'UNSAFE_componentWillMount',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillmount'
)
},
{
message: errorMessage(
'componentWillReceiveProps', '16.3.0', 'getDerivedStateFromProps',
'componentWillReceiveProps', '16.3.0', 'UNSAFE_componentWillReceiveProps',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillreceiveprops'
)
},
{
message: errorMessage('componentWillUpdate', '16.3.0', 'getDerivedStateFromProps',
message: errorMessage('componentWillUpdate', '16.3.0', 'UNSAFE_componentWillUpdate',
'https://reactjs.org/docs/react-component.html#unsafe_componentwillupdate'
)
}
Expand Down