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

bug: multiple concurrent SSR page requests conflict #165

Closed
zenflow opened this issue Mar 28, 2021 · 4 comments · Fixed by #169
Closed

bug: multiple concurrent SSR page requests conflict #165

zenflow opened this issue Mar 28, 2021 · 4 comments · Fixed by #169

Comments

@zenflow
Copy link

zenflow commented Mar 28, 2021

I noticed this while working on another problem. Both have to do with the fact that a single client is shared for all SSR page requests.

This is called inside prepareReactRender() before using rendering the React node asynchronously:
https://github.com/gqless/gqless/blob/df417eace136321ef6ee55a0f19c3eef2e33174e/packages/gqless/src/Helpers/ssr.ts#L78-L86

If I'm not mistaken, if a second request comes in before the first completes, any data already collected for the first will be lost, and one or both of the requests will receive data that they didn't ask for.

For this and other reasons (I will open another issue) I think it would be a lot better if each SSR page request had it's own gqless client.

@PabloSzx
Copy link
Contributor

PabloSzx commented Mar 28, 2021

completely good reasoning, the problem with that is that the clients are not in the react tree, we would have to add them in a React Context tree to be able to differentiate between clients, and that's not possible with the current design, since you are importing from the client module to use "query", "mutation", "resolved", etc. For the react hooks that wouldn't be a problem, but for the core it is.

If you have an implemention idea of how to do it, that would be very helpful

@PabloSzx
Copy link
Contributor

PabloSzx commented Mar 28, 2021

I can think of one option, tell me what do you think about this:

The cache is never cleared, but the stale-while-revalidates are enabled by default, and in the prepareReactRender we also await for those revalidations to complete.

Actually, simply wrapping the prepass in the refetch helper should do the trick, and using the stale-while-revalidates wouldn't work since those are executed after useEffect, and those don't get called on ssr

@zenflow
Copy link
Author

zenflow commented Mar 28, 2021

the problem with that is that the clients are not in the react tree, we would have to add them in a React Context tree to be able to differentiate between clients, and that's not possible with the current design, since you are importing from the client module to use "query", "mutation", "resolved", etc.

@PabloSzx Yup, and I think it would be great to change that! Relates to #166

For the react hooks that wouldn't be a problem, but for the core it is.

I'm not sure what you mean by this. I don't see what problem core has to deal with here.
I think the core createClient/GqlessClient is essentially fine in this issue, but we are using it wrong on the next layer, where we only create one client and use it for all SSR page requests. If we fixed that (instead create & use a different client for each page request), then the code linked above (clearing cache) would be dead code / not needed, right?

If you have an implemention idea of how to do it, that would be very helpful

Please see my recommendation in #166 if you haven't already. We can expand on the proposal there if you are interested.

Actually, simply wrapping the prepass in the refetch helper should do the trick, and using the stale-while-revalidates wouldn't work since those are executed after useEffect, and those don't get called on ssr

If we start with a fresh gqless client for each SSR page request, we don't have the problem I think you're trying to solve (getting cached data to be refetched), .

@PabloSzx
Copy link
Contributor

PabloSzx commented Mar 28, 2021

For now, I will add an immediate fix for the "prepareRender":

  • don't remove the previous cache, simply use the existing "innerState.allowCache" to trigger the requests.
  • don't overwrite the existing cache, simply merge it
  • only pass the "query" cache, not mutation & subscriptions
  • pass the selections aliases for queries with arguments, logic implemented for the persistence feature, and very relevant for this feature as well.
  • lower the snapshot size

PR: #169

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

Successfully merging a pull request may close this issue.

2 participants