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 some polling/StrictMode issues, along with some general refactorings #8414

Merged
merged 19 commits into from
Jun 23, 2021

Conversation

brainkim
Copy link
Contributor

@brainkim brainkim commented Jun 22, 2021

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

Notes

Continuation of #8346

Spent some time deleting Reobserver, as well as fixing some polling misbehavior.

Most of the test changes are related to fixing StrictMode stuff. The timings of moving ObservableQuery logic to useEffect made it so that we need little bits of delay to get the tests working. The behavior doesn’t seem to have changed in any meaningful way, we just skip some renders when we refetch or rerender too soon. There was one test which just didn’t make sense, was never failable in any meaningful sense, and used timer mocks in a way which messed with other tests so I deleted it. Forgive me.

This should probably make Apollo Client “strict mode” safe, though it is roughly impossible to tell for certain.

Fixes

TODO: Investigate if this fixes

@brainkim brainkim force-pushed the brian-the-janitor branch from 88c134e to 9d40bc1 Compare June 22, 2021 05:05
@brainkim brainkim requested review from hwillson and benjamn June 22, 2021 05:13
@hwillson hwillson added this to the June 2021 milestone Jun 22, 2021
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.

Thanks to the granularity of your commits, @brainkim, these changes were actually pretty easy to review! I left a few comments in-line, but nothing merge-blocking.

Speaking of merging, feel free to rebase and squash commits together before merging if you like, but I think I'd prefer to keep these separate commits (with a merge commit), instead of squashing the whole PR.

src/react/data/QueryData.ts Outdated Show resolved Hide resolved
src/core/ObservableQuery.ts Outdated Show resolved Hide resolved
src/react/hooks/__tests__/useQuery.test.tsx Show resolved Hide resolved
src/core/ObservableQuery.ts Show resolved Hide resolved
src/core/ObservableQuery.ts Outdated Show resolved Hide resolved
@brainkim brainkim force-pushed the brian-the-janitor branch from 9d40bc1 to 90b5aeb Compare June 22, 2021 21:07
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

This looks great @brainkim - thanks very much!

(just a gentle reminder to update the CHANGELOG 👍)

@brainkim brainkim force-pushed the brian-the-janitor branch from ec5f909 to 5d1db15 Compare June 23, 2021 15:15
@brainkim brainkim merged commit 606d6e1 into release-3.4 Jun 23, 2021
@benjamn
Copy link
Member

benjamn commented Jun 23, 2021

@brainkim I just released this in @apollo/client@3.4.0-rc.13, fyi.

brainkim added a commit that referenced this pull request Jul 23, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
@brainkim brainkim deleted the brian-the-janitor branch July 23, 2021 23:37
brainkim added a commit that referenced this pull request Jul 27, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Jul 27, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Jul 28, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
@hwillson hwillson removed this from the MM-2021-06 milestone Jul 29, 2021
brainkim added a commit that referenced this pull request Jul 29, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Jul 29, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Jul 31, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Aug 2, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Aug 3, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Aug 4, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Aug 5, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Aug 5, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Aug 5, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Aug 6, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Aug 6, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Aug 6, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Aug 9, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Aug 10, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Aug 11, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Aug 13, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Aug 16, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Aug 16, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
brainkim added a commit that referenced this pull request Aug 16, 2021
After #8414, the
changes made in #6107
are unnneccessary, because all ObservableQuery callbacks will only be
fired in useEffect calls (hopefully). This changes the timings of some
of tests.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
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.

3 participants