Add support for React 16.3 lifecycle methods in sort-comp #1771

dejoean · 2018-04-18T23:43:47Z

Relevant issue: #1767

The new lifecycle methods added in React 16.3 need to be added to the sort-comp rule for compatibility.

Also the rule needs to be updated to allow static method positions to be overridden so that getDerivedStateFromProps can be placed in the ordering that exists in React documentation, which appears to be how the other methods are currently ordered by default (aside from the render method).

Would love to hear any feedback/suggested changes.

I detailed potential issues etc. in my commit notes here:

Add React 16.3 lifecycle methods to default config
Allow static method position to be overriden by specification
- Note: this update means that a static method of the same name as a non-static method in the sort-comp will be considered an error if it is not placed in the position corresponding to its name
Add instance-variables to the lifecycle after the constructor and before getDefaultProps
- Note: This update is not required, but is a nice to have since the default order has to be updated anyway, and current behavior forces all instance variables to come after componentWillUnmount by default, which is a strange ordering.
- Note: The super constructor runs prior to instance variables being instantiated, so this.props can be referenced within an instance variable definition. However, instance variables are defined prior to the current class' constructor executing. Since it is hard to represent this flow I thought it would be better to place instance variables below the constructor so as to make it obvious that this.props is defined, since defining an instance variable then using it in the constructor seems like an unnecessary use case.
Add a test case for a React 16.3 class
Update documentation to reflect changes

- Add React 16.3 lifecycle methods to default config - Allow static method position to be overriden by specification - Note: this update means that a static method of the same name as a non-static method in the sort-comp will be considered an error if it is not placed in the position corresponding to its name - Add `instance-variables` to the lifecycle after the `constructor` and before `getDefaultProps` - Note: This update is not required, but is a nice to have since the default order has to be updated anyway, and current behavior forces all instance variables to come after `componentWillUnmount` by default, which is a strange ordering. - Note: The super constructor runs prior to instance variables being instantiated, so `this.props` can be referenced within an instance variable definition. However, instance variables are defined prior to the current class' constructor executing. Since it is hard to represent this flow I thought it would be better to place instance variables below the constructor so as to make it obvious that `this.props` is defined, since defining an instance variable then using it in the constructor seems like an unnecessary use case. - Add a test case for a React 16.3 class - Update documentation to reflect changes

otakustay · 2018-04-19T03:52:05Z

docs/rules/sort-comp.md

@@ -9,7 +9,7 @@ When creating React components it is more convenient to always follow the same o
 The default configuration ensures that the following order must be followed:

  1. static methods and properties
-  2. lifecycle methods: `displayName`, `propTypes`, `contextTypes`, `childContextTypes`, `mixins`, `statics`,`defaultProps`, `constructor`, `getDefaultProps`, `getInitialState`, `state`, `getChildContext`, `componentWillMount`, `componentDidMount`, `componentWillReceiveProps`, `shouldComponentUpdate`, `componentWillUpdate`, `componentDidUpdate`, `componentWillUnmount` (in this order).
+  2. lifecycle methods: `displayName`, `propTypes`, `contextTypes`, `childContextTypes`, `mixins`, `statics`, `defaultProps`, `constructor`, `instance-variables`, `getDefaultProps`, `state`, `getInitialState`, `getChildContext`, `getDerivedStateFromProps`, `componentWillMount`, `UNSAFE_componentWillMount`, `componentDidMount`, `componentWillReceiveProps`, `UNSAFE_componentWillReceiveProps`, `shouldComponentUpdate`, `componentWillUpdate`, `UNSAFE_componentWillUpdate`, `getSnapshotBeforeUpdate`, `componentDidUpdate`, `componentWillUnmount`, `componentDidCatch` (in this order).


I think componentDidCatch should be placed before componentWillUnmount since it is a method when component is "live"

I agree that it is strange to put it after componentWillUnmount. Where would you recommend putting it? Do you think it would make more sense directly after componentDidMount or directly before componentWillUnmount?

I think part of the reason it was separated from the lifecycle and placed at the bottom in the docs is that it doesn't fall within the update cycle like the other methods between componentDidMount and componentWillUnmount.

fwiw, you can throw in the componentWillUnmount of a child and it will be caught by componentDidCatch.

I'm not sure, but you may even be able to throw an error it would catch from an unmounted child component, assuming you have an unhandled timeout somewhere. Putting componentDidCatch at the end does make it clear that it catches errors from the preceding lifecycle hooks, but then again it doesn't actually catch errors within the component it was defined inside of. I just went with the docs on this one originally since the existing ordering followed them, but I don't know whether they put it at the bottom to separate it documentation-wise or because they would actually suggest writing it at the bottom, since all of the examples I have seen have been error boundary components with no other lifecycle methods.

It will be better to put getDerivedStateFromProps right before static, since it's a static lifecycle.

What I mean is that we should recommend the separation of "self controlled state" and those "derived from props" even they can be written in the form of constructor or class property, this may be a best practice:

getDerivedStateFromProps(nextProps, prevState) { if (prevState.prevousPropValue === nextProps.value) { return { bar: value, // reset other states }; } }

In most case, we should not have different logics between initial and update state compute, one derived state property should always be synced to props

Whether or not those should be separated does not really impact that the initial state declaration happens before getDerivedStateFromProps is fired. In your own example, you would want to set previousPropValue in the initial state declaration based on this.props.value, or else the initial line of getDerivedStateFromProps is misleading, as it makes it seem like you are only updating on changes to the value prop (I assume you meant !==). I would write it like this:

state = { value: this.props.value bar: someExpensiveProcessing(this.props.value), } static getDerivedStateFromProps = (nextProps, prevState) => ({ ...( (prevState.value !== nextProps.value) ? { value: nextProps.value, bar: someExpensiveProcessing(nextProps.value), } : {} ), })

This style may lead to some code duplication, but it ultimately makes the code clearer since the initial state is actually valid.

Also there is not necessarily any complete separation between the state here, as getDerivedStateFromProps receives the previous state, and could choose to only alter a state value if it hadn't been changed already, or a state value might need to be updated locally as well as on prop changes to keep it up to date. For instance:

state = { toggle: this.props.isToggleOn, countFromProps: this.props.extraCount, count: 0, } static getDerivedStateFromProps = (nextProps, prevState) => ({ toggle: prevState.toggle || nextProps.isToggleOn, count: (prevState.count - prevState.countFromProps) + nextProps.extraCount, }) onClick = () => { this.setState((prevState) => ({ toggle: true, count: prevState.count + 1, })) }

My consideration is we should choose to place getDerivedStateFromProps either:

right after constructor if we think it should be responsible to compute the initial state

at the place where componentWillReceiveProps was if we recommend to use it only on prop changes (not initial instantiation)

To be clear I think there is also a use case for the initial state declaration as a set of defaults, like the following

state = { localeString: '0', } static getDerivedStateFromProps = (nextProps, prevState) => ({ localeString: ( !Math.isNaN(Number(nextProps.number)) && nextProps.number.toLocaleString() ) || prevState.localeString, })

since this allows us to easily say that the component will convert the prop number to a locale string if it is a number and cache the value in state, and fall back to displaying the default of '0' or the last valid number that we received. In this case even if we wanted to avoid recalculating the value if the prop hadn't changed by caching the unmodified number prop in state for comparisons, we would have a sensible default of 0 for it since we wouldn't need to recalculate the default localeString if it were 0 initially. Also if there are nested objects in state that you spread out in getDerivedStateFromProps it would make sense to initialize the objects in the state declaration so that additional checks wouldn't have to be written and then executed on each update.

If there is a sensible value for the initial state that isn't derived from props and the code reads clearly I would prefer to avoid setting the initial state based off of props in general, since it is an antipattern as it does not respect prop changes after mounting. I just would not leave out required state values from the initial state declaration as it serves to annotate the expected state fields in a component as well as in some cases default values for those fields.

getDerivedStateFromProps remains a lifecycle method that is used for updates as well as mounting, and that is fired after the state initialization on the initial mount. If we were to place the state declaration below getDerivedStateFromProps in the code above it would appear as though localeString would come through as undefined in the initial prevState based on the ordering, which is not the case.

Put didCatch directly before WillUnmount.

webOS101 · 2018-04-19T22:51:14Z

I think I would prefer to see the instance-variables removed from this PR so it could be debated further. This will cause a lot of previously passing source code to fail (I believe). Keep in mind that many methods are defined as instance variables and should not appear before the lifecycle methods. Further, I don't see any additional tests validating the change to instance-variables. Thanks for putting this together!

dejoean · 2018-04-19T23:58:08Z

@webOS101 The nomenclature is a bit strange on instance-variables as they actually explicitly exclude instance-methods even if instance-methods is not a specified rule. instance-variables only refers to non-static class property instantiations. Essentially the current behavior if you use class properties is for sort-comp to error out unless you define all your class properties after componentWillUnmount since they are caught by the everything-else rule, which is extremely strange, considering that they are essentially equivalent to this.myClassProp = 'my value' in the constructor, and are actually instantiated prior to the local constructor running (but after the super constructor). I will add an instance variable to the test case I added. Let me know if you still think I should separate the pull request though.

webOS101 · 2018-04-20T17:16:35Z

I certainly understand the reasoning and agree that you're probably right about where it belongs. I'm just concerned about migrating an entire code base to the new rule to take advantage of the new lifecycle methods. Also, if we had to revert one or the other parts of this PR, they would be tied together and not easy to separate. Are there code reordering auto-fix rules? I didn't poke deeply but my recollection is that the current auto-fix rules only work on a line at a time.

dejoean · 2018-04-20T18:26:43Z

I don't believe that there is an auto fix in place for this rule unfortunately. I see your point that it could be separated out but I would consider the current behavior a bug and if someone wanted to keep the old behavior due to their code base having been written to that standard that person could copy the default ordering and remove instance-variables from it in their eslintrc. This option seems better to me than potentially exposing new users to broken behavior or requiring users to override the rule to get it into a non-broken state. Also the addition of instance-variables was extremely simple and could be removed very easily if necessary. Mostly I just didn't want to create a potentially broken commit. Just let me know if you want me to pull it out. Thanks for the feedback!

ljharb

Please also add test cases that use actual instance methods - arrow functions in class properties are not instance methods, they're just instance variables, and lifecycle methods should be prototype methods anyways (actual instance methods).

ljharb · 2018-04-22T21:56:51Z

docs/rules/sort-comp.md

@@ -9,7 +9,7 @@ When creating React components it is more convenient to always follow the same o
 The default configuration ensures that the following order must be followed:

  1. static methods and properties
-  2. lifecycle methods: `displayName`, `propTypes`, `contextTypes`, `childContextTypes`, `mixins`, `statics`,`defaultProps`, `constructor`, `getDefaultProps`, `getInitialState`, `state`, `getChildContext`, `componentWillMount`, `componentDidMount`, `componentWillReceiveProps`, `shouldComponentUpdate`, `componentWillUpdate`, `componentDidUpdate`, `componentWillUnmount` (in this order).
+  2. lifecycle methods: `displayName`, `propTypes`, `contextTypes`, `childContextTypes`, `mixins`, `statics`, `defaultProps`, `constructor`, `instance-variables`, `getDefaultProps`, `state`, `getInitialState`, `getChildContext`, `getDerivedStateFromProps`, `componentWillMount`, `UNSAFE_componentWillMount`, `componentDidMount`, `componentWillReceiveProps`, `UNSAFE_componentWillReceiveProps`, `shouldComponentUpdate`, `componentWillUpdate`, `UNSAFE_componentWillUpdate`, `getSnapshotBeforeUpdate`, `componentDidUpdate`, `componentWillUnmount`, `componentDidCatch` (in this order).


Put didCatch directly before WillUnmount.

ljharb · 2018-04-22T21:57:13Z

docs/rules/sort-comp.md

@@ -92,7 +99,7 @@ The default configuration is:
 * `type-annotations`. This group is not specified by default, but can be used to enforce flow annotations positioning.
 * `getters` This group is not specified by default, but can be used to enforce class getters positioning.
 * `setters` This group is not specified by default, but can be used to enforce class setters positioning.
-* `instance-variables` This group is not specified by default, but can be used to enforce all other instance variables positioning.
+* `instance-variables` This group can be used to enforce all other instance variables' positioning.


specifying this by default makes this a breaking change; please revert that piece.

ljharb · 2018-04-22T21:57:42Z

tests/lib/rules/sort-comp.js

+      '  render = () => (<div>Hello</div>)',
+      '}'
+    ].join('\n'),
+    parser: 'babel-eslint'


Please add a test case that parses with the default parser and also covers all the new lifecycle methods.

As far as I know I don't think I can test getDerivedStateFromProps with createReactClass. Is there a way I could specify a static method within the class body? I can write one for the other new methods though.

class Foo {} Foo.getDerivedStateFromProps = … should work fine outside the class body

Ya, I just meant that writing it that way wouldn't test anything with this rule since it wouldn't be sorted within the component body. I checked earlier and if I remember correctly it doesn't get caught by the rule in that case (as one would expect) or in the case where it is placed within statics (which I am not sure is for declaring static methods anyway [since I don't have much experience with the React.createClass style of React]). I can just leave it out though, or preferably add it after the component definition like you mentioned to make sure that doesn't get caught as an error in future versions.

ah true. you could do static getDerivedStateFromProps() { } since it's a method tho?

Oh, for some reason I didn't realize the default parser based tests had support for basic ES6 classes.

you may need to specify the ecmaVersion as 2015 or higher, which is fine

dejoean · 2018-04-22T22:43:43Z

@ljharb Technically the current behavior catches both arrow functions and regular methods as instance-methods interchangeably, and, as I mentioned above, excludes arrow functions from instance-variables (regardless of whether any of these behaviors are desired). I can definitely add a prototype method version to my test as well though. I can also make all of the lifecycle methods prototype methods but technically they can use arrow function syntax and still work so it might be good to test both cases.

ljharb · 2018-04-23T22:14:18Z

Arrow functions are not instance methods though :-/

dejoean · 2018-04-24T18:53:08Z

Does anyone know if this build issue with let being used in Node 4 is related to a change I made, possibly the use of ES6 class style React in a test without the babel-eslint plugin?

ljharb · 2018-04-24T19:24:36Z

To use let in node 4, the file must have 'use strict'; at the top.

dejoean · 2018-04-24T22:34:58Z

I saw that in the error but I wasn't sure if I could add 'use-strict' anywhere in order to fix the issue. I just attempted a fix by adding 'use-strict' before the test using an ES6 class on the default parser, but adding it there didn't change anything. I can try removing that test as well to see if the issue is actually related to it.

dejoean · 2018-04-24T22:47:59Z

Ok it wasn't related to the test, and it in fact seems to be occurring on npm install inside of npm-cli.js.

$ nvm install 4
Downloading and installing node v4.9.1...
Downloading https://nodejs.org/dist/v4.9.1/node-v4.9.1-linux-x64.tar.xz...
Computing checksum with sha256sum
Checksums matched!
Now using node v4.9.1 (npm v2.15.11)
$ node --version
v4.9.1
$ npm --version
2.15.11
$ nvm --version
0.33.9
before_install
$ nvm install-latest-npm
Attempting to upgrade to the latest working version of npm...
* `npm` v2.x needs to first jump to the latest v2 to be able to upgrade further
/home/travis/.nvm/versions/node/v4.9.1/bin/npm -> /home/travis/.nvm/versions/node/v4.9.1/lib/node_modules/npm/bin/npm-cli.js
[email protected] /home/travis/.nvm/versions/node/v4.9.1/lib/node_modules/npm
* Installing latest `npm`; if this does not work on your node version, please report a bug!
/home/travis/.nvm/versions/node/v4.9.1/bin/npx -> /home/travis/.nvm/versions/node/v4.9.1/lib/node_modules/npm/bin/npx-cli.js
/home/travis/.nvm/versions/node/v4.9.1/bin/npm -> /home/travis/.nvm/versions/node/v4.9.1/lib/node_modules/npm/bin/npm-cli.js
[email protected] /home/travis/.nvm/versions/node/v4.9.1/lib/node_modules/npm
* npm upgraded to: v
$ npm install 
/home/travis/.nvm/versions/node/v4.9.1/lib/node_modules/npm/bin/npm-cli.js:79
      let notifier = require('update-notifier')({pkg})

The above shows up in the logs on the Travis CI build, which makes it seem like NPM has pushed an update that has broken compatibility with Node 4. It seems like npm was upgraded to 2.15.12 here but it is hard tell since the "upgraded to" display is broken and has just printed v.

Also the AppVeyor build seems to be using [email protected], which was published 17 hours ago, and it failed on the same statement.

dejoean · 2018-04-24T23:03:43Z

npm/npm@834b46f#diff-fff9baaea61bc82d9999fab7b965272c
This seems to be the commit that introduced the issue. I imagine this update has affected all of the automated builds in this repo since its release ~17 hours ago.

ljharb

Is there an existing test that covers "state" and "getInitialState" in the same component? (not that that would make tons of sense)

ljharb · 2018-04-25T21:11:02Z

docs/rules/sort-comp.md

@@ -70,30 +70,36 @@ The default configuration is:
      'defaultProps',
      'constructor',
      'getDefaultProps',
-      'getInitialState',


this could technically be a breaking change; is there a reason to swap the order of state and getInitialState?

I changed the order in the documentation to match the actual order in the code, I didn't actually change the order in the code. Also the two are mutually exclusive in actual usage so testing them together would be strange, and I wouldn't be sure how to even write that test considering that state in this sense only exists as an instance variable on an ES6 class and getInitialState only exists as a createReactClass method.

Makes perfect sense :-) I didn't notice this was just the docs.

ljharb · 2018-04-25T21:12:36Z

@joe-denea it's now fixed in nvm itself, on travis - I've reran the errored builds.

dejoean · 2018-04-25T21:34:15Z

@ljharb Looks like AppVeyor is still using [email protected] and getting the error unfortunately.

ljharb · 2018-04-26T01:36:02Z

Ah, good call. I'll try to fix that too.

hamlim · 2018-05-08T12:30:14Z

Will there be a new release pushed out soon with this change? We are about to migrate to React 16.3 and this rule, without this change, could start to become a pain for many of our developers/reviewers when using the new lifecycle methods.

yannickcr · 2018-05-09T09:28:31Z

@hamlim yes, it should be released in the next few days.

browne0 · 2018-05-24T22:02:22Z

Just curious this was 16 days ago, when will the next release be coming out? 😄

yannickcr · 2018-05-28T21:27:26Z

@browne0 This feature was was released in 7.8.0 17 days ago 😉

dejoean mentioned this pull request Apr 18, 2018

Update sort-comp for React 16.3+ #1767

Closed

otakustay reviewed Apr 19, 2018

View reviewed changes

ljharb requested changes Apr 22, 2018

View reviewed changes

dejoean force-pushed the master branch 2 times, most recently from bb5e382 to 5dbf7e3 Compare April 24, 2018 22:29

dejoean force-pushed the master branch from 795c5e2 to 5dbf7e3 Compare April 24, 2018 22:38

ljharb reviewed Apr 25, 2018

View reviewed changes

stunstunstun mentioned this pull request Apr 26, 2018

I am confused about the lifecycle sequence in 16.3 airbnb/javascript#1792

Closed

ljharb force-pushed the master branch from 5dbf7e3 to 768013e Compare April 26, 2018 07:29

ljharb approved these changes Apr 26, 2018

View reviewed changes

ljharb added enhancement react 16 labels Apr 26, 2018

ljharb merged commit fb7ce94 into jsx-eslint:master Apr 26, 2018

ljharb mentioned this pull request May 14, 2018

Sort-comp rule doesn't enforce getDerivedStateFromProps placement #1793

Closed

Add support for React 16.3 lifecycle methods in sort-comp #1771

Add support for React 16.3 lifecycle methods in sort-comp #1771

Conversation

dejoean commented Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

dejoean Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yenshih Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dejoean Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dejoean Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webOS101 commented Apr 19, 2018 • edited Loading

dejoean commented Apr 19, 2018 • edited Loading

webOS101 commented Apr 20, 2018 • edited Loading

dejoean commented Apr 20, 2018 • edited Loading

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

dejoean Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dejoean Apr 24, 2018 • edited Loading

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

dejoean commented Apr 22, 2018 • edited Loading

ljharb commented Apr 23, 2018

dejoean commented Apr 24, 2018

ljharb commented Apr 24, 2018

dejoean commented Apr 24, 2018

dejoean commented Apr 24, 2018 • edited Loading

dejoean commented Apr 24, 2018 • edited Loading

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dejoean Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Apr 25, 2018

dejoean commented Apr 25, 2018 • edited Loading

ljharb commented Apr 26, 2018

hamlim commented May 8, 2018

yannickcr commented May 9, 2018

browne0 commented May 24, 2018

yannickcr commented May 28, 2018

dejoean commented Apr 18, 2018 •

edited

Loading

dejoean Apr 19, 2018 •

edited

Loading

yenshih Apr 20, 2018 •

edited

Loading

dejoean Apr 20, 2018 •

edited

Loading

dejoean Apr 20, 2018 •

edited

Loading

webOS101 commented Apr 19, 2018 •

edited

Loading

dejoean commented Apr 19, 2018 •

edited

Loading

webOS101 commented Apr 20, 2018 •

edited

Loading

dejoean commented Apr 20, 2018 •

edited

Loading

dejoean Apr 22, 2018 •

edited

Loading

dejoean Apr 24, 2018 •

edited

Loading

dejoean commented Apr 22, 2018 •

edited

Loading

dejoean commented Apr 24, 2018 •

edited

Loading

dejoean commented Apr 24, 2018 •

edited

Loading

dejoean Apr 25, 2018 •

edited

Loading

dejoean commented Apr 25, 2018 •

edited

Loading