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

Remove the warning for setState on unmounted components #22114

Merged
merged 2 commits into from
Aug 18, 2021

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 17, 2021

We have a warning that fires when you setState on 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:

useEffect(() => {
  function handleChange() {
     setState(store.getState())
  }
  store.subscribe(handleChange)
  return () => store.unsubscribe(handleChange)
}, [])

If you forget the unsubscribe call, 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:

async function handleSubmit() {
  setPending(true)
  await post('/someapi') // component might unmount while we're waiting
  setPending(false)
}

This will trigger the setState warning. 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:

let isMountedRef = useRef(false)
useEffect(() => {
  isMountedRef.current = true
  return () => {
    isMountedRef.current = false
  }
}, [])

async function handleSubmit() {
  setPending(true)
  await post('/someapi')
  if (isMountedRef.current) {
    setPending(false)
  }
}

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 because isMountedRef.current is false while 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.warn instead of console.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 useMutableSource where appropriate anyway.

So the proposal is to stop compromising the common product case, and not warn at all.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Aug 17, 2021
@sizebot
Copy link

sizebot commented Aug 17, 2021

Comparing: bd25570...72278da

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 127.58 kB 127.58 kB = 40.72 kB 40.72 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 130.40 kB 130.40 kB = 41.65 kB 41.65 kB
facebook-www/ReactDOM-prod.classic.js = 405.16 kB 405.16 kB = 75.04 kB 75.04 kB
facebook-www/ReactDOM-prod.modern.js = 393.72 kB 393.72 kB = 73.32 kB 73.32 kB
facebook-www/ReactDOMForked-prod.classic.js = 405.16 kB 405.16 kB = 75.05 kB 75.05 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactDOM-dev.classic.js = 1,062.03 kB 1,059.44 kB = 235.38 kB 234.94 kB
facebook-www/ReactDOMForked-dev.classic.js = 1,062.03 kB 1,059.44 kB = 235.38 kB 234.94 kB
facebook-www/ReactDOM-dev.modern.js = 1,037.58 kB 1,035.00 kB = 230.42 kB 229.98 kB
facebook-www/ReactDOMForked-dev.modern.js = 1,037.58 kB 1,035.00 kB = 230.42 kB 229.98 kB
oss-experimental/react-dom/umd/react-dom.development.js = 1,032.94 kB 1,030.36 kB = 223.59 kB 223.17 kB
oss-experimental/react-dom/cjs/react-dom.development.js = 983.20 kB 980.74 kB = 220.99 kB 220.56 kB
oss-stable-semver/react-dom/umd/react-dom.development.js = 1,014.14 kB 1,011.56 kB = 219.84 kB 219.42 kB
oss-stable/react-dom/umd/react-dom.development.js = 1,014.14 kB 1,011.56 kB = 219.84 kB 219.42 kB
oss-stable-semver/react-dom/cjs/react-dom.development.js = 965.35 kB 962.89 kB = 217.24 kB 216.81 kB
oss-stable/react-dom/cjs/react-dom.development.js = 965.35 kB 962.89 kB = 217.24 kB 216.81 kB
facebook-www/ReactDOMTesting-dev.classic.js = 970.17 kB 967.59 kB = 217.94 kB 217.54 kB
facebook-www/ReactDOMTesting-dev.modern.js = 943.10 kB 940.52 kB = 212.29 kB 211.88 kB
oss-experimental/react-art/umd/react-art.development.js = 767.60 kB 765.02 kB = 162.12 kB 161.66 kB
oss-stable-semver/react-art/umd/react-art.development.js = 750.88 kB 748.30 kB = 158.68 kB 158.22 kB
oss-stable/react-art/umd/react-art.development.js = 750.88 kB 748.30 kB = 158.68 kB 158.22 kB
oss-experimental/react-reconciler/cjs/react-reconciler.development.js = 714.98 kB 712.52 kB = 151.88 kB 151.46 kB
react-native/implementations/ReactNativeRenderer-dev.fb.js = 737.20 kB 734.62 kB = 159.37 kB 158.95 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.development.js = 698.91 kB 696.45 kB = 148.44 kB 148.02 kB
oss-stable/react-reconciler/cjs/react-reconciler.development.js = 698.91 kB 696.45 kB = 148.44 kB 148.02 kB
react-native/implementations/ReactFabric-dev.fb.js = 722.06 kB 719.48 kB = 156.03 kB 155.58 kB
react-native/implementations/ReactNativeRenderer-dev.js = 719.60 kB 717.02 kB = 156.22 kB 155.77 kB
facebook-www/ReactART-dev.classic.js = 711.61 kB 709.03 kB = 151.79 kB 151.32 kB
react-native/implementations/ReactFabric-dev.js = 702.45 kB 699.87 kB = 152.28 kB 151.83 kB
facebook-www/ReactART-dev.modern.js = 701.33 kB 698.75 kB = 149.69 kB 149.21 kB
oss-experimental/react-art/cjs/react-art.development.js = 663.77 kB 661.31 kB = 143.76 kB 143.31 kB
oss-stable-semver/react-art/cjs/react-art.development.js = 647.89 kB 645.43 kB = 140.34 kB 139.89 kB
oss-stable/react-art/cjs/react-art.development.js = 647.89 kB 645.43 kB = 140.34 kB 139.89 kB
oss-experimental/react-test-renderer/umd/react-test-renderer.development.js = 647.21 kB 644.63 kB = 136.25 kB 135.84 kB
oss-experimental/react-test-renderer/cjs/react-test-renderer.development.js = 617.02 kB 614.56 kB = 134.77 kB 134.37 kB
oss-stable-semver/react-test-renderer/umd/react-test-renderer.development.js = 630.52 kB 627.94 kB = 132.83 kB 132.41 kB
oss-stable/react-test-renderer/umd/react-test-renderer.development.js = 630.52 kB 627.94 kB = 132.83 kB 132.41 kB
oss-stable-semver/react-test-renderer/cjs/react-test-renderer.development.js = 601.17 kB 598.71 kB = 131.39 kB 130.98 kB
oss-stable/react-test-renderer/cjs/react-test-renderer.development.js = 601.17 kB 598.71 kB = 131.39 kB 130.98 kB
facebook-www/ReactTestRenderer-dev.modern.js = 623.17 kB 620.59 kB = 133.62 kB 133.19 kB
facebook-www/ReactTestRenderer-dev.classic.js = 623.16 kB 620.58 kB = 133.61 kB 133.18 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-dev.js = 611.70 kB 609.12 kB = 132.38 kB 131.97 kB

Generated by 🚫 dangerJS against 72278da

@@ -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', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@quisido
Copy link

quisido commented Aug 17, 2021

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?

They tend to be concentrated in libraries, which eventually would likely move to useMutableSource where appropriate anyway.

Should this wait until that hook is generally available?

@callmeberzerker
Copy link

callmeberzerker commented Aug 18, 2021

The amount of isMounted() custom hooks that gonna be deleted from React projects it's gonna be huge -> and yes this was so annoying and it was false positive 99% of the time.

Most projects have this isMounted() hook only to avoid that warning - or the good ol console.error switch-a-roo:

const KNOWN_VIOLATIONS = [
'Warning: Can't perform a React state update on an unmounted'
]

export const configureConsoleError = (silenceKnownErrors = true): void => {
  if (silenceKnownErrors) {
    const oldError = console.error
    console.error = (...args) => {
      const firstArg = args[0]
      if (typeof firstArg === 'string' && KNOWN_VIOLATIONS.some((v) => firstArg.includes(v))) {
        return
      }
      oldError(...args)
    }
  }
}

Huge props for making this (and please include it in the Patch notes). 🍻

@gaurav5430
Copy link
Contributor

So the warning is being removed because

  • false positives in common use cases
  • the isMounted solution would not work well for future use cases

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):
isn't this a valid pattern?

Working with class components, we had this understanding that a good pattern was to handle api calls / data fetching in componentDidUpdate collectively instead of being handled individually in event listeners, specially when you had to do something AFTER the async state update, even if it is in an event listener.
The same thing when translated to hooks would mean handling that in useEffect instead of event listeners.
This is difficult with hooks because of the extra dependency management overhead, but isn't that one of the suggested patterns anymore?

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 18, 2021

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.

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.

personally I haven't seen that POST use case in any of the projects I have worked with

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.

but I have seen the "less common" use case a lot (mostly because there is no easy way to unsubscribe to api calls/promises)

The "less common" case (a store subscription) has no relation to API calls or Promises. What do you mean?

Working with class components, we had this understanding that a good pattern was to handle api calls / data fetching in componentDidUpdate collectively instead of being handled individually in event listeners, specially when you had to do something AFTER the async state update, even if it is in an event listener.

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(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we are not asserting anything here now, wouldn't this test pass even if the actual implementation DID throw a warning ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, our infra catches unexpected warnings.

@gaurav5430
Copy link
Contributor

gaurav5430 commented Aug 18, 2021

The "less common" case (a store subscription) has no relation to API calls or Promises. What do you mean?

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 get api call which sets component state in .then

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 .then

isn't that understanding correct ?

@gaurav5430
Copy link
Contributor

gaurav5430 commented Aug 18, 2021

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.

Apologies, the second part of my comment about the data fetching was based on my first observation (explained in the previous comment), the get api call / data fetch in componentDidMount.

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:

  • firing a tracking event on component mount (mutation in lifecycle)
  • changing the page title on component mount (mutation in lifecycle)
  • fetching some data on button click (data fetching on user action)
  • fetching data when the component mounts (data fetching in lifecycle)
  • submitting a form on user action (mutation on user action)

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) ?
(I am just trying to understand why should we treat them differently)

(just to be clear, I am NOT suggesting to not remove the warning, just trying to understand better the motivation behind it)

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 18, 2021

For example, a get api call which sets component state in .then

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 .then

Well, there's two things here:

  1. After the Promise resolves, it (if you don't have other leaks) will be garbage collected together with your component's object. Presumably your then() won't last forever.
  2. But this is already a problem today anyway! Just adding if (!isMounted) does not unsubscribe you from anything. In fact there is no way to "unsubscribe" from a Promise. So what you're describing is already how the code that tries to avoid the warning works today. For these cases, the warning is not effective in any way. And like I said earlier, there's no particular concern because it only lives as long as the fetch itself.

The only case for which the warning is useful is when you have an API like addEventListener + removeEventListener. This is not the common case for which this warning tends to show up.

@gaurav5430
Copy link
Contributor

But this is already a problem today anyway! Just adding if (!isMounted) does not unsubscribe you from anything. In fact there is no way to "unsubscribe" from a Promise. So what you're describing is already how the code that tries to avoid the warning works today. For these cases, the warning is not effective in any way. And like I said earlier, there's no particular concern because it only lives as long as the fetch itself.

yeah, this is a problem,

  • people tend to notice this problem because of the existing warning for unmounted setState
  • the isMounted solution is not valid in this case, but we can use implementation specific promise cancellation to unsubscribe (?)

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.
Though it may not be for the lifetime of the app (which I wasn't really sure of, because of limited understanding of how promise garbage collection exactly works, but would take your word for it), it is still a memory leak. This is my most common use case for this warning.

May be not as much a concern as a store.subscribe or event listeners, which are valid for the lifetime of the app, so yeah, fine I guess.

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 18, 2021

but we can use implementation specific promise cancellation to unsubscribe (?)

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.

but possibly not, who knows what else is referencing that promise.
Though it may not be for the lifetime of the app (which I wasn't really sure of, because of limited understanding of how promise garbage collection exactly works, but would take your word for it), it is still a memory leak. This is my most common use case for this warning.

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.

@gaurav5430
Copy link
Contributor

gaurav5430 commented Aug 18, 2021

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:

  • the data fetching or any promise based use case is either rare enough, not valid (as the memory leak,if any, would generally be for a short time) or not (easily) solvable, so this warning does not really contribute to the solution.
  • there are some actual / valid use cases like store subscriptions, (they can be abstracted and solved at coomon places) and the drawbacks or antipatterns due to this warning far outweigh them

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 18, 2021

Sorry, I realize my bold might have come across as shouting -- just trying to highlight the main point.

the data fetching or any promise based use case is either rare enough

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.

not valid (as the memory leak,if any, would generally be for a short time) or not (easily) solvable, so this warning does not really contribute to the solution.

Yes.

there are some actual / valid use cases like store subscriptions, (they can be abstracted and solved at coomon places) and the drawbacks or antipatterns due to this warning far outweigh them

Yes, that's my perception.

@NMinhNguyen
Copy link
Contributor

NMinhNguyen commented Aug 21, 2021

  if (!isMountedRef.current) {
    setPending(false)
  }

Small correction, but in the event handler code, I think it's meant to be if (isMountedRef.current) (without the exclamation mark).

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Aug 24, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.