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 executing standby queries in ssr #9382

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

atarek12
Copy link

@atarek12 atarek12 commented Jan 31, 2022

close #9336
close #9108

I tried this and all is working as expected. The issue was the promise is not resolved and keep loading forever whenever the fetch policy was standby.

@apollo-cla
Copy link

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

@atarek12 atarek12 force-pushed the fix-ssr-standby-queries branch from c545a57 to db57b90 Compare February 1, 2022 05:37
@atarek12 atarek12 marked this pull request as draft February 1, 2022 06:31
@atarek12 atarek12 force-pushed the fix-ssr-standby-queries branch from db57b90 to 278bc0b Compare February 1, 2022 08:07
@atarek12 atarek12 marked this pull request as ready for review February 1, 2022 08:14
@brainkim brainkim requested review from brainkim and benjamn February 1, 2022 20:31
@atarek12
Copy link
Author

atarek12 commented Feb 2, 2022

@brainkim @benjamn you can address this example to find the issue

@atarek12
Copy link
Author

atarek12 commented Feb 2, 2022

I have added tests for this case

@atarek12 atarek12 force-pushed the fix-ssr-standby-queries branch 2 times, most recently from 8ee2964 to 7433b2e Compare February 8, 2022 08:48
@atarek12
Copy link
Author

@brainkim @benjamn any updates?

@WaveDashShine
Copy link

is this merge request still being looked at?
I'm running into issue #9108
Is there a workaround for useLazyQuery in the meantime?

@atarek12
Copy link
Author

atarek12 commented Mar 2, 2022

is this merge request still being looked at? I'm running into issue #9108 Is there a workaround for useLazyQuery in the meantime?

The current workaround is to set ssr: false in the query options

@atarek12 atarek12 force-pushed the fix-ssr-standby-queries branch from d4583df to c7a29b1 Compare April 14, 2022 11:59
@atarek12
Copy link
Author

atarek12 commented Apr 14, 2022

I have reverted the main branch changes to get all tests green, but here are some failed tests due to outdated docs.

@atarek12 atarek12 force-pushed the fix-ssr-standby-queries branch 2 times, most recently from 0961593 to 4bd8d76 Compare April 22, 2022 02:04
@jpvajda jpvajda added the 🏓 awaiting-contributor-response requires input from a contributor label May 3, 2022
@atarek12 atarek12 force-pushed the fix-ssr-standby-queries branch from 4bd8d76 to 7393757 Compare May 4, 2022 16:36
@atarek12
Copy link
Author

atarek12 commented May 4, 2022

  • have pulled the new apollo client version
  • adding a small fix when using useQuery hook with fetchPolicy = "standby" in SSR mode
  • adding test cases for that

@jpvajda jpvajda added 🏓 awaiting-team-response requires input from the apollo team and removed 🏓 awaiting-contributor-response requires input from a contributor labels May 5, 2022
@atarek12
Copy link
Author

atarek12 commented Jun 3, 2022

@jpvajda any updates related to this PR?

@jpvajda jpvajda requested review from jpvajda and removed request for brainkim June 3, 2022 14:25
@jpvajda
Copy link
Contributor

jpvajda commented Jun 3, 2022

@atarek12 Apologies for delay here! I'm going to tag @benjamn on the review. We've done a decent amount of work around useQuery so I want to ensure this PR won't create any conflicts before merging.

@atarek12
Copy link
Author

atarek12 commented Jun 6, 2022

I have posted a reproduction case for that in the issue and I am gonna repost it here too

here is my reproduction case, clone this repo and run yarn and yarn dev

@baygeldin
Copy link

Thanks for this PR, it helped a lot in understanding why the getDataFromTree method was getting stuck all the time for no reason. There was not a single query in the codebase that used the standby fetch policy, and yet it was stuck resolving promises for queries that had fetchPolicy: "standby" set somehow, so I figured it was added automatically somewhere. I did some digging and sure enough, if a query uses skip option it sets fetch policy to standby by default. And it does it in such a way that this fix doesn't actually work 😢

So, instead I've applied the following patch (using pnpm patch command, but one could use patch-package as well):

diff --git a/react/ssr/ssr.cjs b/react/ssr/ssr.cjs
index 1241a6767609da0d1ef224b8f9f2ef2cec35147c..ace8f0adcb5dabfd982cef09507119a79d4edf5b 100644
--- a/react/ssr/ssr.cjs
+++ b/react/ssr/ssr.cjs
@@ -61,6 +61,7 @@ var RenderPromises = (function () {
         return finish ? finish() : null;
     };
     RenderPromises.prototype.addObservableQueryPromise = function (obsQuery) {
+        if (obsQuery.options.fetchPolicy == 'standby') return null
         return this.addQueryPromise({
             getOptions: function () { return obsQuery.options; },
             fetchData: function () { return new Promise(function (resolve) {

Make sure to patch the right file (react/ssr/ssr.cjs, not react/hooks/useQuery.js). Server-side bundles use different modules than those on the client side.

@baygeldin
Copy link

Also, semantically speaking I don't think that force skipping SSR for standby queries makes sense. These queries still need to make a network request during SSR and populate the cache (it's not cache-only after all). The problem is that RenderPromises relies on the ObservableQuery to notify it when data is fetched, but the whole point of standby queries is that they don't notify anything so that they don't trigger re-render of components that use them.

Currently, the workaround is to treat any query with skip option set carefully and don't use the standby fetch policy when using SSR. And just in case apply the aforementioned patch so that your server doesn't hang resolving promises that ain't gonna resolve anyway.

So, all in all this is a really nasty bug and I don't think a simple fix would work here because the issue runs deeper. Although maybe @benjamn could shed some light on a possible solution since it's related to SSR 🙏

@jpvajda jpvajda requested review from jerelmiller and removed request for benjamn and jpvajda November 7, 2022 18:50
@jpvajda
Copy link
Contributor

jpvajda commented Nov 7, 2022

@jerelmiller @bignimbus - This PR might be relevant to some of the work we are doing to offer better SSR support, could you both take a look when you have a free moment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: useQuery with a fetch policy of standby causes SSR to hang useLazyQuery brakes the whole app
5 participants