Eslint-plugin-react: consider removing no-did-mount-set-state from recommended rules

[jedwards1211]

Yes it leads to layout thrashing, but what else can you do when you need to perform a JS-computed layout? One can work around this by using DOM measuring wrapper components, but for components that do JS layout, setState() in componentDidMount() is a fairly common way of doing it.

[yannickcr]

I am agree with you, actually I'm doing exactly the same thing in some of my projects (render, measure, setState, re-render).

But since it is pretty easy to do some accidental layout trashing without even realizing it I think it is better to keep this rule in the recommended ones, and use a /* eslint-disable react/no-did-mount-set-state */ comment when justified.

[mjackson]

It's absolutely necessary to do a setState inside componentDidMount for doing things like measuring. Since there's no other way to do this, I'd agree with @jedwards1211 that it should be removed from the "recommended" rules.

@yannickcr Maybe it's time for a react/warnings config? eslint-plugin-import does this and it's great. You simply extend import/warnings if you want to see these kinds of opinionated error messages and import/errors if you only want to see actual error cases. Could be a solution to #636 as well.

[jedwards1211]

Well one _could_ dispatch Redux actions and store layout info in Redux state, and it will be interesting to see if anyone comes up with a really slick fully-JS layout solution for React components, but when one just needs a few workarounds for the limitations of CSS layout, then component state is a really logical place to put the layout info.

[jedwards1211]

Of course that begs the question: eslint leaders wouldn't consider dispatching Redux actions in componentDidMount a code smell, I hope?

[mjackson]

"Despatch a Redux action" is just a really elaborate way of setting state.
On Sat, Jun 18, 2016 at 4:25 PM Andy Edwards [email protected]
wrote:

Of course that begs the question: eslint leaders wouldn't consider
dispatching Redux actions in componentDidMount a code smell, I hope?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/yannickcr/eslint-plugin-react/issues/596#issuecomment-226970476,
or mute the thread
https://github.com/notifications/unsubscribe/AAFqpzd4z-hbv2ujAiMY1qWyD4msy7kSks5qNH5UgaJpZM4IdPGc
.

[mjackson]

@yannickcr If you think react/warnings and react/errors might be a good approach, I can make a PR with what I think would be appropriate. Interested?

[jedwards1211]

@mjackson my point exactly

[yannickcr]

@mjackson that's an interesting approach, even if I mostly agree with @ljharb about warnings https://github.com/yannickcr/eslint-plugin-react/pull/636#issuecomment-226657532.

I'll be definitely interested if you have the time to craft a PR with this.

[ddevault]

Another point: async componentDidMount functions.

[yannickcr]

@SirCmpwn I'm not sure about what you are referring to. Should not the allow-in-func option cover this case?

[ddevault]

That option seems to be designed for this case, but it doesn't work with async/await keywords from ES7. Example:

async componentDidMount() {
    const thing = await foobar();
    this.setState({ thing });
}

[ljharb]

(nit: async/await is not in ES7, it's stage 3 and just a "future ES proposal")

[ddevault]

Nit sufficiently picked, still an issue :)

[yannickcr]

Since there is some legitimate use cases to use setState here I have removed these rules from the recommended configuration. The rules are still available for who want to activate them manually.