Eslint-plugin-react: Rule proposal: react/no-will-mount

[jacobrask]

There are few use cases for componentWillMount that cannot be handled in the constructor.

It is a common mistake to put asynchronous requests in componentWillMount which could potentially cause hard to find errors.

[ljharb]

I disagree; i think everything that's not initializing state belongs in componentWillMount.

Async requests in the constructor have the same issues; neither us often a problem in practice.

I'm a strong -1.

[jacobrask]

Wouldn't most async requests in practice set state on completion?

From my POV the question here is if there are a reasonable number of developers considering putting asynchronous requests in componentWillMount a problem, and that most other use cases can be handled in the constructor.

I think the React docs imply this.

Avoid introducing any side-effects or subscriptions in this method.
...
Generally, we recommend using the constructor() instead.

My understanding (which is very limited, so please correct me if I'm wrong) is that async rendering with fibers means that an async request might complete before mount and sometimes after (maybe the mount could even be aborted?).

(I would be willing to implement this if I can be reasonably sure that it would be accepted.)

[ljharb]

Side effects in JS never belong in constructors. If React advises that, they are wrong.

[jacobrask]

My quotation was a bit confusing, those were excerpts from different paragraphs.

[ljharb]

Indeed, fiber may change the timing of some of these things. But fiber isn't out yet, and if the issue is doing async things prior to the first render, then constructor vs componentWillMount has the same problem.

In other words, I'm still not seeing a reason to discourage componentWillMount - you've only talked about async setStates initiated prior to mounting.

[lukeapage]

tracking: https://github.com/facebook/react/issues/7671

[jacobrask]

It seems like a significant number of React developers agree that using this method should be discouraged. If you do not agree it's of course fine to not use the rule.

Are you opposed to (me?) adding the rule at all to this project?

[ljharb]

I'm not sure where you get "significant number" - that github issue is full of people who simply don't see a need for the method, with very very few claiming that its use should be discouraged.

So yes, I'm opposed to adding the rule at all.

[jacobrask]

Ok

[ljharb]

I'm not the only collaborator; this should remain open pending the others' thoughts.

[abritinthebay]

Given it's an official FB recommendation (quoted above)

Generally, we recommend using the constructor() instead.

It would make sense to have this as an optional linting rule. Certainly shouldn't be a recommended one but a lot of the core devs seem pro-deprecation and have mentioned that post-fiber it would make sense to examine the issue more.

Most linting rules are completely subjective, this seems to at least be a reasonable suggestion for an optional rule.

[ljharb]

Afaik there's still active discussions going on with core devs about it; I don't believe they've made a compelling case. Note that Facebook doesn't server-render, and their position on componentWillMount is one that is not shared by other stakeholders that do server-render.

[abritinthebay]

Sure, there are plenty of active discussions about many things - including things where rules already exist in this package.

That's what best practice debates are composed of 😄

My point is more - this kind of rule is a _classic_ optional linting step feature. God knows there are existing rules in this package that I'm _never_ going to turn on because they don't work for me or my projects. This rule would be no different to those.

Side note: I extensively use server-rendering and honestly, the story there is not as complex as some in that discussion would make it. Though yes, there are complexities involved and for those components that _must_ use cWM then perhaps turning the rule off makes sense - but that's good, as it requires intentionality.

[ljharb]

My comment stands, as does this one.

[abritinthebay]

Indeed, your preference is clear. If the rule existed you'd not use it/turn it off. 👍

[ljharb]

To clarify; my preference is to not include the rule, so that nobody is ever able to turn it on.

[noahgrant]

this is a different context/discussion and a different rule, but now that React 16.3 is going to be deprecating cWM, cWRP and cWU, i'd love to have a linting rule that we could put in place to prevent using these methods as we migrate away from them. should we create a new discussion for it?

[ljharb]

It’s not yet possible to recommend alternatives for every use case, nor have they actually been deprecated yet.

When React provides enough replacements for cWM etc, then we’ll definitely make linting rules to cover them.

[Hypnosphi]

Here they are: https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html

[Hypnosphi]

Generally, it would make sense to me to have a forbid-methods rule, where one could list the methods that shouldn't be used

[abritinthebay]

Oh that's a quite nice generalized rule, could even have a recommended/default set.

[ljharb]

This seems handled by #1750.