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.

Sign up for GitHub

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

Jump to bottom

fix: address React StrictMode error when query resolves after unmount #6216

Merged
merged 2 commits into from
Jun 2, 2020
Merged

fix: address React StrictMode error when query resolves after unmount #6216

merged 2 commits into from
Jun 2, 2020

Conversation

grxy
Copy link
Contributor

@grxy grxy commented Apr 29, 2020 •
edited
Loading

Motivation

useQuery can log a warning in React's StrictMode when a component is unmounted before the query has been resolved.

Changes

  • Adds a failing test to demonstrate the issue
  • Checks if a component is mounted before running the forceUpdate function

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

Resolves apollographql/react-apollo#3635

@apollo-cla
Copy link

@grxy: 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/

@grxy grxy marked this pull request as ready for review April 29, 2020 20:45
@benjamn
Copy link
Member

benjamn commented May 8, 2020

@grxy Would you mind rebasing?

@benjamn benjamn requested a review from hwillson May 8, 2020 14:57
@nodabladam nodabladam mentioned this pull request May 23, 2020
Closed
@nodabladam
Copy link

I believe I am experiencing the same problem. It would be great to get this into apollo 3

@rajohan
Copy link

rajohan commented May 25, 2020

Got the same problem, tried your solution by manually making the changes in the node_modules folder and it fixes it. Would be nice to get this added.

benjamn
benjamn approved these changes Jun 2, 2020
View reviewed changes
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.

Looks good, and thanks for rebasing @grxy!

@benjamn benjamn merged commit c76804e into apollographql:master Jun 2, 2020
@jvrbnjak89 jvrbnjak89 mentioned this pull request Jun 2, 2020
Open
@joonhocho joonhocho mentioned this pull request Jun 4, 2020
Closed
@joonhocho
Copy link

joonhocho commented Jun 4, 2020 •
edited
Loading

This seems to break SSR. #6386 . Just spent a good amount of time tracking down the bug, and this makes an app to hang indefinitely.

benjamn added a commit that referenced this pull request Jun 4, 2020
@benjamn
Allow onNewData to be called when unmounted, during SSR.
cbab9a9 
PR #6216 introduced these isMounted guards, and was merged just before the
v3.0.0-rc.0 release. It appears (see #6386) that requiring the component
to be mounted is too restrictive, specifically when doing server-side
rendering, since the concept of mounting doesn't make sense without a DOM.
@benjamn benjamn mentioned this pull request Jun 4, 2020
Merged
benjamn added a commit that referenced this pull request Jun 4, 2020
@benjamn
Allow onNewData to be called when unmounted, during SSR. (#6388)
b6da9c8 
PR #6216 introduced these isMounted guards, and was merged just before the
v3.0.0-rc.0 release. It appears (see #6386) that requiring the component
to be mounted is too restrictive, specifically when doing server-side
rendering, since the concept of mounting doesn't make sense without a DOM.
benjamn added a commit that referenced this pull request Jun 5, 2020
@benjamn
Revert #6388 and #6216, fixing #6386 and #6385.
58cad04 
This reverts commits b6da9c8 (#6388) and
c76804e (#6216).

Preventing StrictMode warnings in React is not worth introducing bugs that
break applications. We will need to reconsider how best to prevent
warnings for unmounted components.
@benjamn benjamn mentioned this pull request Jun 5, 2020
Merged
benjamn added a commit that referenced this pull request Jun 5, 2020
@benjamn
Revert #6388 and #6216, fixing #6386 and #6385. (#6395)
41fb5fe 
This reverts commits b6da9c8 (#6388) and
c76804e (#6216).

Preventing StrictMode warnings in React is not worth introducing bugs that
break applications. We will need to reconsider how best to prevent
warnings for unmounted components.
@benjamn benjamn mentioned this pull request Jun 5, 2020
Closed
@Nightbr Nightbr mentioned this pull request Jan 21, 2021
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@benjamn benjamn benjamn approved these changes

@hwillson hwillson Awaiting requested review from hwillson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

useQuery causes warning in strict mode
6 participants
@grxy @apollo-cla @benjamn @nodabladam @rajohan @joonhocho