Remove the warning for setState on unmounted components #22114
Conversation
|
Comparing: bd25570...72278da Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
| @@ -1401,7 +1359,7 @@ describe('ReactHooksWithNoopRenderer', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| it('warns about state updates for unmounted components with no pending passive unmounts', () => { | |||
| it('does not warn about state updates for unmounted components with no pending passive unmounts', () => { | |||
There was a problem hiding this comment.
I could maybe delete these tests but I figured if we want to adjust the behavior again it's nice to have them in the negated form. Even though they're a bit too specific now.
|
The worst case scenario today seems to be, "adding slightly more runtime overhead, slightly more code, and making the code slightly less readable — all to avoid an effectively false positive warning," but the worst case scenario after this change seems to be an increase in memory leaks. Is that the trade-off one really wants?
Should this wait until that hook is generally available? |
|
The amount of Most projects have this Huge props for making this (and please include it in the |
|
So the warning is being removed because
and the solution for the less common use cases is to useMutableSource Question: how was the common use case determined? personally I haven't seen that POST use case in any of the projects I have worked with but I have seen the "less common" use case a lot (mostly because there is no easy way to unsubscribe to api calls/promises) This will also fix the issue where people move their mutations to effect due to this warning, as it adds unnecessary dependency management, and may lead to firing the mutation multiple times. Question (might be slightly unrelated): Working with class components, we had this understanding that a good pattern was to handle api calls / data fetching in |
I'd say the worst case is people putting mutations in effects (and then have them over-fire), which we're seeing quite widely. This warning alone probably won't save you from a memory leak either. In practice they're often subtle and hard to diagnose without special expertise. What's worse, it seems like this warning has given many developers a wrong idea of what a memory leak even is.
I'm not sure if there is a misunderstanding here. If you work on any project where a user can modify data (create a post, send a message, update an item, buy a product), there's likely code roughly like this in the event handler. It doesn't have to even literally be a POST request, that's just an example. Any mutation would work the same way and run into this problem.
The "less common" case (a store subscription) has no relation to API calls or Promises. What do you mean?
None of the cases discussed in this thread have any relation to data fetching. They're about mutations — sending a form, posting a message, and so on. Mutations belong in the event handlers, while regular data fetching, like you say, usually belongs in lifecycles/effects. Nothing has changed here. |
| expect(renders).toBe(2); | ||
|
|
||
| ReactDOM.render(<div />, container); | ||
|
|
||
| expect(() => { |
There was a problem hiding this comment.
since we are not asserting anything here now, wouldn't this test pass even if the actual implementation DID throw a warning ?
There was a problem hiding this comment.
No, our infra catches unexpected warnings.
i wasn't talking about the examples that you provided, was talking about the less common case more generally, where you subscribe to something ( store, promise) For example, a This is a subscription, which the component should unsubscribe to on unmount, and it is a valid use case for having this warning, as the component instance is kept in memory because of using setState in isn't that understanding correct ? |
Apologies, the second part of my comment about the data fetching was based on my first observation (explained in the previous comment), the I considered async operations and mutations to be treated similarly in extension the examples you provided, because mutations and async operations both may be related to lifecycle or user actions. For example:
considering the above, are you suggesting that some of the cases would remain as is (data fetching), some of them would be solved better without this warning (mutations on user action) ? (just to be clear, I am NOT suggesting to not remove the warning, just trying to understand better the motivation behind it) |
Well, there's two things here:
The only case for which the warning is useful is when you have an API like |
yeah, this is a problem,
as you said, possibly the promise gets garbage collected soon because no one else is referencing it, but possibly not, who knows what else is referencing that promise. May be not as much a concern as a |
I don’t know what you mean by this. Promises don’t support cancellation. Things like AbortController, to the best of my knowledge, don’t solve the memory “leak” problem. And regardless there is no leak here because the Promise eventually resolves.
I’m afraid we’re starting to go in circles. What you say is true but the warning did not help solve this “leak” anyway because there is no way for you to solve it aside from moving away from Promises. |
|
Yeah, sorry, I was not trying to prove a point. Just stating publicly to get feedback on my understanding. My observation related to this warning was mostly around this particular usage: api call subscription and unsubscribe which we use a lot in our codebase, and hence this whole conversation. To sum up my understanding, from this conversation it seems that:
|
|
Sorry, I realize my bold might have come across as shouting -- just trying to highlight the main point.
On the contrary, I'd say it's very common! But in this use case, the "leak" neither lasts a long time, nor is it easy to solve anyway without significantly changing the approach.
Yes.
Yes, that's my perception. |
Small correction, but in the event handler code, I think it's meant to be |
Summary: Post: https://fb.workplace.com/groups/rnsyncsquad/permalink/879923262900946/ This sync includes the following changes: - **[fc3b6a411](facebook/react@fc3b6a411 )**: Fix a few typos ([#22154](facebook/react#22154)) //<Bowen>// - **[986d0e61d](facebook/react@986d0e61d )**: [Scheduler] Add tests for isInputPending ([#22140](facebook/react#22140)) //<Andrew Clark>// - **[d54be90be](facebook/react@d54be90be )**: Set up test infra for dynamic Scheduler flags ([#22139](facebook/react#22139)) //<Andrew Clark>// - **[7ed0706d7](facebook/react@7ed0706d7 )**: Remove the warning for setState on unmounted components ([#22114](facebook/react#22114)) //<Dan Abramov>// - **[9eb2aaaf8](facebook/react@9eb2aaaf8 )**: Fixed ReactSharedInternals export in UMD bundle ([#22117](facebook/react#22117)) //<Brian Vaughn>// - **[bd255700d](facebook/react@bd255700d )**: Show a soft error when a text string or number is supplied as a child to non text wrappers ([#22109](facebook/react#22109)) //<Sota>// Changelog: [General][Changed] - React Native sync for revisions 424fe58...bd5bf55 jest_e2e[run_all_tests] Reviewed By: yungsters Differential Revision: D30485521 fbshipit-source-id: c5b92356e9e666eae94536ed31b8de43536419f8
We have a warning that fires when you
setStateon an unmounted components. This is a proposal to remove it.Why was this warning added?
The warning states that it protects against memory leaks. The original use case (rewritten to Hooks) goes like this:
If you forget the
unsubscribecall, after the component unmounts you'll get a memory leak. So we warn you if you set state after unmounting because we think you might've forgotten to do that.Why does this warning create problems?
The problem is that in practice, there's usually a much more common case in the user code:
This will trigger the
setStatewarning. However, there is no actual problem here. There's no "memory leak" here: the POST comes back, we try to set state, the component is unmounted (so nothing happens), and then we're done. Nothing keeps holding onto the component indefinitely.Avoiding false positives is too difficult
So effectively the above case is a false positive but that's the most common one people try to solve. Even the "canonical" solution to this is pretty clumsy:
It's unfortunate because we're adding slightly more runtime overhead, slightly more code, and making the code slightly less readable — all to avoid an effectively false positive warning.
Bu there are other issues, too.
In the future, we'd like to offer a feature that lets you preserve DOM and state even when the component is not visible, but disconnect its effects. With this feature, the code above doesn't work well. You'll "miss" the
setPending(false)call becauseisMountedRef.currentisfalsewhile the component is in a hidden tab. So once you return to the hidden tab and make it visible again, it will look as if your mutation hasn't "come through". By trying to evade the warning, we've made the behavior worse.In addition, we've seen that this warning pushes people towards moving mutations (like POST) into effects just because effects offer an easy way to "ignore" something using the cleanup function and a flag. However, this also usually makes the code worse. The user action is associated with a particular event — not with the component being displayed — and so moving this code into effect introduces extra fragility. Such as the risk of the effect re-firing when some related data changes. So the warning against a problematic pattern ends up pushing people towards more problematic patterns though the original code was actually fine.
Could we not remove this?
We could make the warning less aggressive. For example,
console.warninstead ofconsole.error. However, as long as it shows up, the institutional knowledge will push people towards using a ref or moving it to an effect, and neither is desirable.We could make add some threshold to the warning. Such as only firing it after N setStates or M seconds. However, it's hard to find one-size-fit-all heuristics, and it's probably going to be confusing either way.
In practice, direct subscriptions in components are not that common anymore. Especially now that we have custom Hooks. They tend to be concentrated in libraries, which eventually would likely move to
useMutableSourcewhere appropriate anyway.So the proposal is to stop compromising the common product case, and not warn at all.