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/destructuring-assignment ignoreClassFields does not support nested objects #1947

Closed
MrHen opened this issue Aug 19, 2018 · 9 comments · Fixed by #1983
Closed

react/destructuring-assignment ignoreClassFields does not support nested objects #1947

MrHen opened this issue Aug 19, 2018 · 9 comments · Fixed by #1983

Comments

@MrHen
Copy link
Contributor

MrHen commented Aug 19, 2018

Using this rule:

        "react/destructuring-assignment": [
            "warn",
            "always",
            {
                "ignoreClassFields": true
            }
        ],

This code:

export default class Thing extends React.Component {
    state = {
        alpha: false,
        beta: this.props.beta,
        charlie: null,
    };
    // ...
}

Generates this warning:

[eslint] Must use destructuring props assignment (react/destructuring-assignment)

Version information:

$ npm ls eslint
@centriam/cx-client@1.3.3 /Users/adam.babcock/Source/cxlint/client
├── eslint@4.16.0
├─┬ eslint-nibble@4.2.1
│ └── eslint@4.16.0  deduped
└─┬ quill-image-resize-module@3.0.0
  └── eslint@3.19.0  extraneous

$ npm ls eslint-plugin-react
@centriam/cx-client@1.3.3 /Users/adam.babcock/Source/cxlint/client
├── eslint-plugin-react@7.11.1
└─┬ quill-image-resize-module@3.0.0
  └── eslint-plugin-react@6.10.3  extraneous
@ljharb
Copy link
Member

ljharb commented Aug 19, 2018

I think that this is a bug, but also, you shouldn’t ever be using props in initial constructor state - you want getDerivedStateFromProps. Otherwise when props change, you might forget to update “beta”

@MrHen
Copy link
Contributor Author

MrHen commented Aug 19, 2018

@ljharb Agreed. I'm going through existing code applying lint fixes and this situation popped up. The whole pattern in this particular file needs to be rewritten / refactored but for now I'm focusing on lint issues.

@alexzherdev
Copy link
Contributor

What about uncontrolled components though? If the component accepts a "defaultValue" prop, I think it only needs to be "translated" to state once on initialization, and after that the value is managed via component's state.

@ljharb
Copy link
Member

ljharb commented Aug 20, 2018

That’d be an edge case, and even then I’d say the component should rerender when it gets an updated prop.

@Jmenache
Copy link

Whenever I'm in the case of having to use props in state definition, I avoid the warning by using the constructor.

export default class Thing extends React.Component {
    constructor(props) {
      super(props);

      this.state = {
          alpha: false,
          beta: props.beta,
          charlie: null,
      };
    }
    // ...
}

About getDerivedStateFromProps, the following React post recommends avoiding it most of the time. One of the preferred solutions uses props in state definition (Fully uncontrolled component with a key)
https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html

@ljharb
Copy link
Member

ljharb commented Sep 12, 2018

@Jmenache note that your pattern is not correct unless you also use setState in componentWillReceiveProps - and the replacement for that pair is indeed to use gDSFP.

@Jmenache
Copy link

@ljharb I think the post recommends using a key instead of componentWillReceiveProps with this pattern, so that it creates a new component and goes through the constructor again.

@ljharb
Copy link
Member

ljharb commented Sep 12, 2018

Certainly that's another workaround, but that seems much less performant and clean.

@Jmenache
Copy link

@ljharb As counter intuitive as it is, the article explains that it is usually not slower, and sometimes faster.

Note
While this may sound slow, the performance difference is usually insignificant. Using a key can even be faster if the components have heavy logic that runs on updates since diffing gets bypassed for that subtree.

I agree it is not pretty and I would most likely try using the other recommended solution if I have a choice, but this pattern helped me a few times dealing with legacy code.

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

Successfully merging a pull request may close this issue.

4 participants