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 extra useQuery result frames #9599

Merged
merged 24 commits into from
Apr 18, 2022
Merged

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Apr 14, 2022

This PR builds on #9508, fixing the unexpected frame by transitioning to a loading: true result synchronously when variables change or the query is refetched.

Since we do not want to commit to making any network requests synchronously during rendering, I have come up with a new way to block all network requests, by passing a fetchBlockingPromise: Promise<boolean> option that intercepts a part of the request pipeline that is already safely asynchronous.

As a happy side effect, this fetchBlockingPromise technique should also prevent making any network requests for extra ObservableQuery objects created during development when using React's <StrictMode>, since the extra component renders are never mounted, and thus useEffect never fires, leaving the fetchBlockingPromise unresolved until it times out and the fetch gets silently canceled (as desired).

FritsvanCampen and others added 20 commits April 14, 2022 14:46
This option allows delaying/canceling any/all network traffic for the
given query, and should prevent making stray network requests on behalf
of useQuery queries whose useEffect callbacks never fire.
This removes the UNNEEDED_FRAME from the second test added in PR #9508,
which currently causes the test to fail, though the version without the
UNNEEDED_FRAME is how we ultimately want useQuery to behave.

I believe the other test is correct to have the repeated frame:
#9508 (comment)
Closes #9508, now that its tests are both accurate and passing.
@dylanwulf
Copy link
Contributor

I have confirmed that this fixes #9549 and it might also fix #9145 and #9580

@benjamn benjamn force-pushed the fix-extra-useQuery-result-frame branch from 375d668 to 8292081 Compare April 18, 2022 19:52
@benjamn benjamn force-pushed the fix-extra-useQuery-result-frame branch from 8292081 to 58cab4e Compare April 18, 2022 19:59
src/core/watchQueryOptions.ts Show resolved Hide resolved
src/core/QueryManager.ts Show resolved Hide resolved
src/core/QueryManager.ts Show resolved Hide resolved
@brainkim
Copy link
Contributor

I think this could be merged for the sake of the release candidate but we should definitely loop around on the fetchBlockingPromise API because it is technically public.

A surgical strike by @benjamn.

cc @hwillson @jpvajda

@benjamn benjamn merged commit 59e12ca into release-3.6 Apr 18, 2022
@brainkim brainkim deleted the fix-extra-useQuery-result-frame branch April 19, 2022 17:37
@yusufumac
Copy link

@benjamn I came across to bug using fetchMore function which is indirectly caused by fetchBlockingPromise. Because resultsFromLink (source) can't execute query - by your design-, fetchQuery function returns undefined in fetchMore causes exception here.

@benjamn
Copy link
Member Author

benjamn commented Apr 21, 2022

@yusufumac Thank you! Would you mind opening a new issue, ideally with a small reproduction? I have some guesses about what the problem might be, but it would be great to see some concrete code.

Are you saying resultsFromLink remains permanently blocked when you use fetchMore? Is it possible the component in question is never actually mounted? I ask because <React.StrictMode> generates double renderings and discards half of them without running useEffect callbacks, which could potentially be a source of components that never mount, so their fetchBlockingPromises resolve to false and stay that way permanently (or until fetchBlockingPromise is removed from the options).

Does any of that sound familiar/plausible from your debugging?

benjamn added a commit that referenced this pull request Apr 28, 2022
WatchQueryOptions.fetchBlockingPromise was introduced recently in #9599,
and probably needed more time to bake on the release-3.6 branch before
the final v3.6.0 release.

This commit removes fetchBlockingPromise from the public
WatchQueryOptions API, but continues passing an equivalent
Promise<boolean> when calling observable.reobserve (whenever the
useQuery options change).

This means we no longer block fetches for the initial call to useQuery,
but that should already be handled (even in <React.StrictMode>) by
waiting until the ObservableQuery gets its very first subscriber (in
useEffect) to initiate the first network request.

Since options.fetchBlockingPromise was intended for internal use and was
never documented, I am hopeful we still have time to remove it in a
patch version (likely 3.6.1).
@benjamn
Copy link
Member Author

benjamn commented Apr 28, 2022

@yusufumac An update relevant to your concerns: PR #9636 removes the fetchBlockingPromise logic, and was just released in @apollo/[email protected] (still tagged as next rather than latest on npm for now). Please let us know if you have a chance to try npm i @apollo/client@next in your reproduction!

@yusufumac
Copy link

@benjamn Thanks for the update. It was a busy week, so I couldn't create a reproducible case for you. I downgraded to 3.5.10 and didn't have the same issue. I will try 3.6.1 and will let you know if the issue still there.

@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants