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

fix: setState after unmount #7655

Merged
merged 11 commits into from
Feb 9, 2021

Conversation

DylanVann
Copy link
Contributor

I'm seeing this error intermittently when running React in strict mode:

image

From the stack trace it seems to be coming from here, where the forceUpdate does not check if the component has unmounted.

Related issue: #6209

Checklist:

  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

@apollo-cla
Copy link

@DylanVann: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

Copy link
Contributor

@jcreighton jcreighton left a comment

Choose a reason for hiding this comment

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

@DylanVann Appreciate the work here but while these changes will resolve the error, it creates an issue for server-side rendering. We call forceUpdate during SSR but as useEffect hasn't fired yet, mounted.current will be false and forceUpdateUnsafe won't fire.

@DylanVann
Copy link
Contributor Author

Thank you @jcreighton, I've changed it to use the original forceUpdate during SSR and the safe version for CSR, hopefully that works better 🤞

Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

LGTM (with a CHANGELOG.md note)! 🚀

@jcreighton jcreighton merged commit 4b9a521 into apollographql:main Feb 9, 2021
benjamn added a commit that referenced this pull request Feb 15, 2021
PR #7655 changed this code so that the Promise.resolve().then(forceUpdate)
branch was not taken when queryDataRef.current was falsy, so forceUpdate()
was called immediately instead (the else branch).

As #7713 demonstrates, the Promise delay is important to avoid calling
forceUpdate() synchronously during the initial render, which can lead to
an infinite rendering loop.

To preserve the intent of #7655, we now do the queryDataRef.current check
inside the Promise callback, since the component could have been unmounted
before the callback fired.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants