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

react/sort-comp: getDerivedStateFromProps #1850

Closed
brandonburkett opened this issue Jul 2, 2018 · 7 comments
Closed

react/sort-comp: getDerivedStateFromProps #1850

brandonburkett opened this issue Jul 2, 2018 · 7 comments

Comments

@brandonburkett
Copy link

After updating to v17, I am getting 'getDerivedStateFromProps should be placed after componentDidUpdate`. I want to say previously in v16 the rule was below the constructor... does the new placement make sense?

@ljharb
Copy link
Collaborator

ljharb commented Jul 2, 2018

Yes - the bug was in eslint-plugin-react (you'd see the change on v16 as well, with a fully updated eslint-plugin-react). Specifically, gDSFP is both a static method and a lifecycle method, and the sorting logic was wrong.

See jsx-eslint/eslint-plugin-react#1795 / jsx-eslint/eslint-plugin-react#1793

@ljharb ljharb closed this as completed Jul 2, 2018
@brandonburkett
Copy link
Author

@ljharb

Apologies! I do not see gDSFP here at all though (assuming this overrides what eslint-plugin-react does?)

https://github.com/airbnb/javascript/blob/master/packages/eslint-config-airbnb/rules/react.js

// Enforce component methods order
    // https://github.com/yannickcr/eslint-plugin-react/blob/843d71a432baf0f01f598d7cf1eea75ad6896e4b/docs/rules/sort-comp.md
    'react/sort-comp': ['error', {
      order: [
        'static-methods',
        'instance-variables',
        'lifecycle',
        '/^on.+$/',
        'getters',
        'setters',
        '/^(get|set)(?!(InitialState$|DefaultProps$|ChildContext$)).+$/',
        'instance-methods',
        'everything-else',
        'rendering',
      ],
      groups: {
        lifecycle: [
          'displayName',
          'propTypes',
          'contextTypes',
          'childContextTypes',
          'mixins',
          'statics',
          'defaultProps',
          'constructor',
          'getDefaultProps',
          'getInitialState',
          'state',
          'getChildContext',
          'componentWillMount',
          'componentDidMount',
          'componentWillReceiveProps',
          'shouldComponentUpdate',
          'componentWillUpdate',
          'componentDidUpdate',
          'componentWillUnmount',
        ],
        rendering: [
          '/^render.+$/',
          'render'
        ],
      },
    }],

@ljharb
Copy link
Collaborator

ljharb commented Jul 2, 2018

Ooh, good call - we do override the defaults.

I think the change (which would still have shown up in v16) is that gDSFP in "statics" vs "lifecycle" changed.

In other words, gDSFP moved from "everything-else" to "statics" in eslint-plugin-react. We could/should add it to lifecycle, but that would still result in it appearing above the constructor.

@brandonburkett
Copy link
Author

Okay cool. I was in refactor mode with my npm updated and was hoping not to have to move em around again too much.

Do you think the order will change at all for 17.x (even if it is just to match the react plugin)?

Internally debating if I delay the class method shuffle or not.

Appreciate all the feedback and thanks for your time on my isssues / questions today!

@ljharb
Copy link
Collaborator

ljharb commented Jul 3, 2018

I do not expect the ordering to change further (although there may be new React lifecycle methods added to the "lifecycle" group)

@brandonburkett
Copy link
Author

To be clear, you do see it moving to statics right? Not below componentDidUpdate (my current error, plugin is updated).

@ljharb
Copy link
Collaborator

ljharb commented Jul 3, 2018

Since lifecycle is primarily for instance methods, i think it would be in "statics"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants