Adjusted no-deprecated rule for React 16.3.0 #1750

sergei-startsev · 2018-03-29T23:28:27Z

Added warnings for deprecated lifecycle methods:

componentWillMount,
componentWillReceiveProps,
componentWillUpdate.

See details React 16.3.0.

Added warnings for componentWillMount, componentWillReceiveProps, componentWillUpdate

ljharb

Thanks, this looks great!

ljharb · 2018-04-01T16:47:03Z

lib/rules/no-deprecated.js

@@ -73,6 +76,10 @@ module.exports = {
      deprecated[`${pragma}.PropTypes`] = ['15.5.0', 'the npm module prop-types'];
      // 15.6.0
      deprecated[`${pragma}.DOM`] = ['15.6.0', 'the npm module react-dom-factories'];
+      // 16.3.0
+      deprecated.componentWillMount = ['16.3.0'];


can we supply messages in all these three, that ideally suggest replacements and link to the react docs on the deprecation?

Could you take a look at the new message pattern?

ljharb · 2018-04-01T16:47:21Z

lib/rules/no-deprecated.js

@@ -37,7 +40,7 @@ module.exports = {
    schema: []
  },

-  create: function(context) {
+  create: Components.detect((context, components, utils) => {


good change

ljharb · 2018-04-01T16:48:37Z

tests/lib/rules/no-deprecated.js

+    {
+      code: 'React.DOM.div',
+      errors: [{
+        message: 'React.DOM is deprecated since React 15.6.0, use the npm module react-dom-factories instead'


can all these tests be migrated to use the errorMessage helper as well?

sure thing, see the updated tests

…ustments

ljharb · 2018-04-01T22:41:33Z

lib/rules/no-deprecated.js

+      // 16.3.0
+      deprecated.componentWillMount = [
+        '16.3.0',
+        'constructor',


hmm, i'm actually not comfortable recommending the constructor here - side-effecting things should never go in a constructor, and there's still some use cases for componentWillMount that aren't covered by "constructor" or "getDerivedStateFromProps".

I wonder if maybe we should also add an "ignore" array of method names; ie so I can configure the rule to ignore componentWillMount but warn on the rest.

How about recommending componentDidMount() (or UNSAFE_componentWillMount() as a temporary measure)?

I guess the unsafe one; but it'd still probably be a good idea to add an ignore list.

So for the sake of consistency this PR should deprecate the 3 life-cycle methods in favor of their UNSAFE_* counterparts and a future PR should deprecate the unsafe methods completely (after React 17)? Correct?

Agree, in terms of consistency it would be better to deprecated them in favor of UNSAFE_ methods, it should allow to avoid confusion. @ljharb, does it sound reasonable to leave warnings with UNSAFE_ methods as new methods for now and deprecate them in 17.0.0?

Regarding the ignore list for that rule, I vote for adding a dedicated issue for further discussion. I could take a look at it.

A separate issue for the "UNSAFE_" methods would be ideal.

…thods in favor of `UNSAFE_*`

sergei-startsev · 2018-04-04T21:31:52Z

@ljharb, is there anything else that we have to discuss here?

ljharb

Thanks, this looks great - I'll rebase and fixup my formatting nits prior to merging.

ljharb · 2018-04-10T04:31:40Z

lib/rules/no-deprecated.js

        const reactModuleName = getReactModuleName(node);
        const isRequire = node.init && node.init.callee && node.init.callee.name === 'require';
        const isReactRequire =
          node.init && node.init.arguments &&
          node.init.arguments.length && typeof MODULES[node.init.arguments[0].value] !== 'undefined'
-        ;
+          ;


this line should be de-indented

ljharb · 2018-04-10T04:31:49Z

lib/rules/no-deprecated.js

    // --------------------------------------------------------------------------
    // Public
    // --------------------------------------------------------------------------

    return {

-      MemberExpression: function(node) {
+      MemberExpression: function (node) {


please revert all unrelated formatting changes in this PR

sahrens · 2018-06-12T23:12:17Z

lib/rules/no-deprecated.js

        'https://reactjs.org/docs/react-component.html#unsafe_componentwillmount'
      ];
      deprecated.componentWillReceiveProps = [
        '16.3.0',
-        'getDerivedStateFromProps',
+        'UNSAFE_componentWillReceiveProps',


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

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

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.

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

@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.

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).

There're no deprecation warnings regarding using UNSAFE_ methods in strict mode, there is 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.

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.

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.

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

gaearon · 2018-11-28T20:43:01Z

While I appreciate the good intent behind this change, I think it was a mistake and has caused some damage. To be clear, it's our mistake that we haven't engaged with this plugin's maintainers to ensure it matches our strategy. Let's try not to repeat this mistake.

It's a big problem that people still see this rule and think those methods are already "deprecated". It makes our messaging inconsistent and, worse, spams people's consoles. It makes updating React seem like a difficult thing to do. A lot of people think ESLint messages are React messages, and just pick a popular config that includes this rule.

In React, we prefer runtime warnings first so that we can effectively coalesce them. We put great care into our runtime warnings, and include links to migration instructions and codemods. This rule has no link to migration instructions, so people assume they need to rename methods by hand. Its documentation includes no upgrade instructions whatsoever.

Again, it's our fault we didn't look into this before, but it's a huge cost to the community who aren't necessarily educated about codemods. This reflected badly on React ecosystem.

In the future, I'd like to propose that:

PRs that add deprecation warnings aren't merged without a look from somebody on the React team.
Deprecation lint warnings aren't introduced until we actually deprecate a feature in React (which hasn't quite happened yet).
Deprecation lint warning messages must include links to codemods and migration instructions.
Deprecation rule documentation page must include links to codemods and migration instructions.
Deprecation lint rule is not added to "recommended" preset until the next React major. In other words, we want people to see React runtime warnings first, and update at their own pace. They can they opt into checking deprecations with a linter. But we want React runtime warnings to be the primary mechanism, and the linter being an opt-in way to avoid regressing. It should never be the case that somebody picks a popular or "recommended" preset, and that preset contains warnings we haven't even had a chance to surface at runtime in React.

Hope this makes sense! Again, I'm thankful for everyone's effort on this — but we can do better to educate the community and avoid a bad perception of complex upgrade paths. It pains me to think of all the beginners who had their terminals spammed with hundreds of warnings and no accessible explanation about e.g. codemods.

gaearon · 2018-11-28T20:52:56Z

I suggest the following plan (I can do it myself):

Change no-deprecations doc page to include a list of deprecations (as separate subheaders), and links to recommended migration path instructions for each of them.
Link each method warning to the relevant section of no-deprecations rule.
Ensure recommended preset doesn't yet include these particular deprecations (since they're not even deprecated inside React yet).

sergei-startsev · 2018-11-28T21:02:20Z

@gaearon I'm thinking about linting these legacy lefecycle methods with a separate rule, e.g. no-legacy. Thoughts?

kof · 2018-11-28T21:09:27Z

@sergei-startsev probably better to call them specifically, e.g. no-class-component etc. Those things are not officially legacy yet.

sergei-startsev · 2018-11-28T21:17:30Z

Just curious why is it beneficial to see React runtime warnings first? I'm not sure if it's expected to see leaked warnings in production without mechanism to catch them in build time.

sergei-startsev · 2018-11-28T21:20:44Z

@kof take a look at Dan's comment, they're legacy for now.

kof · 2018-11-28T21:23:03Z

Ah sorry, did see this one, but still configuring specific stuff is nicer, more explicit and we don't need to have this terminology, which might mean different things over time.

ljharb · 2018-11-28T21:28:15Z

@gaearon Thanks for your comment!

Regarding your suggestions:

PRs that add deprecation warnings aren't merged without a look from somebody on the React team.

Sounds good! How should I best ping you all?

Deprecation lint warnings aren't introduced until we actually deprecate a feature in React (which hasn't quite happened yet).

That sounds reasonable, but even a runtime warning is imo a deprecation in React.

Deprecation lint warning messages must include links to codemods and migration instructions.
Deprecation rule documentation page must include links to codemods and migration instructions.

Sounds great; a PR to update any existing docs/messages is most welcome.

Deprecation lint rule is not added to "recommended" preset until the next React major. In other words, we want people to see React runtime warnings first, and update at their own pace. They can they opt into checking deprecations with a linter. But we want React runtime warnings to be the primary mechanism, and the linter being an opt-in way to avoid regressing. It should never be the case that somebody picks a popular or "recommended" preset, and that preset contains warnings we haven't even had a chance to surface at runtime in React.

This unfortunately isn't feasible, because eslint requires the entire rule be enabled or disabled wholesale - and we don't want to make separate deprecation rules for each version of react. Certainly enabling any rule in a preset is a semver-major change, as should be causing any additional warnings by enabling an option.

I'm not sure I agree that runtime warnings should be the primary mechanism; runtime warnings are incredibly easy to miss, suppressed in production, and the linter is a far more effective way (in a large organization, at least) to get attention on these warnings.

sergei-startsev · 2018-11-28T21:41:51Z

@ljharb, @gaearon what are our next steps regarding legacy methods? Would like to move it to a dedicated rule, adjust the existing? I can pick up the task if u specify the direction.

gaearon · 2018-11-30T00:09:16Z

I'm thinking about linting these legacy lefecycle methods with a separate rule, e.g. no-legacy. Thoughts?

Another option would be for no-deprecated to take React version as an argument. These methods could be tagged as something like 16.999 until we've decided which release deprecates them. People who want to opt in can specify 17 if they want to see the messages.

Additionally, there should probably be a strict-mode rule which replicates the warnings currently emitted by <StrictMode> and similarly take a version as argument.

I'm not sure if it's expected to see leaked warnings in production without mechanism to catch them in build time.

Not sure what you mean about seeing them in production. React warnings are DEV-only. You'd see them during development. Their only purpose is to warn about removals in future majors — so their presence also doesn't mean your production code is buggy. Unlike many other rules.

Just curious why is it beneficial to see React runtime warnings first?

A few reasons.

The first is that we coalesce runtime warnings where possible. We can show a single warning for many components. We try to avoid showing the same warning twice (if we do, it's usually a bug).

The second is that you only see warnings for code that actually runs while you develop. This may seem like it's bad. (Don't we want to find all issues?) But the all-or-nothing approach often discourages people from doing the migration work. If you see all warnings at once (instead of your little corner of the product) you're less incentivized to fix them. So we consider limiting your initial exposure to just the parts you interact with to be a feature, not a bug.

Of course once you fixed them (or at least some of them) you want to avoid regressing. That's where the rule comes it. I think lint rules are helpful for preventing regressions, but aren't the best instrument to get notified about the issues for the first time. At least with the open source tools today that just show you all lint warnings at once.

Sounds good! How should I best ping you all?

GH notifications in other repos are too noisy but filing a React issue or pinging in Twitter DM are good ways.

That sounds reasonable, but even a runtime warning is imo a deprecation in React.

We don't have runtime warnings for these methods in React outside of Strict Mode.

I'm not sure I agree that runtime warnings should be the primary mechanism; runtime warnings are incredibly easy to miss, suppressed in production, and the linter is a far more effective way (in a large organization, at least) to get attention on these warnings.

I agree with that for new code. The thing I'm worried about is upgrade path. You bump a preset everybody uses, and suddenly it says you have 500 violations. Even if you're on an older React minor that doesn't consider this feature deprecated yet. I think that's pretty confusing.

sergei-startsev · 2018-11-30T00:34:41Z

Another option would be for no-deprecated to take React version as an argument. These methods could be tagged as something like 16.999 until we've decided which release deprecates them. People who want to opt in can specify 17 if they want to see the messages.

I'm concerned about version 17 in lint configuration, it can confuse... As alternative I'd propose to move it to the existing no-unsafe rule since componentWillMount, componentWillReceiveProps, componentWillUpdate also cause Unsafe lifecycle methods warnings in strict mode. @gaearon does it sound reasonable to you?

If you see all warnings at once (instead of your little corner of the product) you're less incentivized to fix them.

Sometimes it's also useful to see the entire scope. If you decide to turn on a new lint rule, you usually expect to see warnings/errors. no-deprecated is a bad example here since it's included into recommended preset, however it should work fine with no-unsafe.

gaearon · 2018-11-30T00:41:34Z

As alternative I'd propose to move it to the existing no-unsafe rule since componentWillMount, componentWillReceiveProps, componentWillUpdate also cause Unsafe lifecycle methods warnings in strict mode.

👍

sergei-startsev · 2018-11-30T00:46:06Z

🤝 if you don't mind I'll request a code review for PR

gaearon · 2018-11-30T18:14:30Z

Note: writing the doc page is still on my todo list but I'm going on a vacation soon so maybe I'll come back to this after.

ljharb · 2018-11-30T23:37:37Z

I think it'd be a bit weird to have a second place to configure a React version - at the very least, it sounds like there's a bunch of deprecations in the rule that are activating at React versions that are lower than they should be - a PR to fix that (even if it hoists the warnings to 16.999) would be appreciated.

Adjusted no-deprecated rule for React 16.3.0

5ab7a23

Added warnings for componentWillMount, componentWillReceiveProps, componentWillUpdate

sergei-startsev mentioned this pull request Mar 29, 2018

Add no-deprecated-methods rule to prevents usage of deprecated component lifecycle methods #1748

Closed

ljharb requested changes Apr 1, 2018

View reviewed changes

ljharb added the enhancement label Apr 1, 2018

Added refs field to refer to docs inno-deprecated rule, minor adj…

501edbf

…ustments

ljharb reviewed Apr 1, 2018

View reviewed changes

Changed warnings in no-deprecated rule, deprecated old lifecycle me…

7034ffc

…thods in favor of `UNSAFE_*`

ljharb approved these changes Apr 10, 2018

View reviewed changes

ljharb added the react 16 label Apr 10, 2018

ljharb force-pushed the no-deprecated branch from 3a8b365 to 7034ffc Compare April 10, 2018 04:40

ljharb mentioned this pull request Apr 10, 2018

Rule proposal: react/no-will-mount #1147

Closed

ljharb merged commit d68ef96 into jsx-eslint:master Apr 10, 2018

yannickcr mentioned this pull request May 9, 2018

Add support for React 16 #1442

Closed

ljharb mentioned this pull request May 12, 2018

semver policy for rule changes #1787

Closed

dependencies bot mentioned this pull request Jun 1, 2018

Update eslint-plugin-react in / from 7.4.0 to 7.8.2 leonardofaria/leozera-ui#197

Closed

sahrens reviewed Jun 12, 2018

View reviewed changes

This was referenced Jun 17, 2018

Add no-unsafe rule #1830

Closed

Added no-unsafe rule #1831

Merged

stepancar mentioned this pull request Jun 26, 2018

Feat/drop componentWillReceiveProps styleguidist/react-styleguidist#1030

Merged

This comment has been minimized.

Sign in to view

sergei-startsev mentioned this pull request Dec 2, 2018

Disable legacy lifecycle methods linting in scope of no-deprecated rule for now #2069

Merged

renovate bot mentioned this pull request Dec 5, 2018

Update dependency eslint-plugin-react to v7 Automattic/jetpack#10827

Closed

1 task

mesosphere-web mentioned this pull request Dec 19, 2018

chore(deps): update dependency eslint-plugin-react to v7 mesosphere/reactjs-components#475

Closed

1 task

Adjusted no-deprecated rule for React 16.3.0 #1750

Adjusted no-deprecated rule for React 16.3.0 #1750

Conversation

sergei-startsev commented Mar 29, 2018

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asapach Apr 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergei-startsev commented Apr 4, 2018

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergei-startsev Jun 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Nov 28, 2018 • edited

gaearon commented Nov 28, 2018

sergei-startsev commented Nov 28, 2018

kof commented Nov 28, 2018

sergei-startsev commented Nov 28, 2018

sergei-startsev commented Nov 28, 2018

kof commented Nov 28, 2018 • edited

ljharb commented Nov 28, 2018 • edited

This comment has been minimized.

This comment has been minimized.

sergei-startsev commented Nov 28, 2018

gaearon commented Nov 30, 2018

sergei-startsev commented Nov 30, 2018 • edited

gaearon commented Nov 30, 2018

sergei-startsev commented Nov 30, 2018

gaearon commented Nov 30, 2018

ljharb commented Nov 30, 2018

asapach Apr 1, 2018 •

edited

sergei-startsev Jun 13, 2018 •

edited

gaearon commented Nov 28, 2018 •

edited

kof commented Nov 28, 2018 •

edited

ljharb commented Nov 28, 2018 •

edited

sergei-startsev commented Nov 30, 2018 •

edited