Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

ssr: false queries are being executed on the server #3130

Closed
hwillson opened this issue Jun 17, 2019 · 26 comments
Closed

ssr: false queries are being executed on the server #3130

hwillson opened this issue Jun 17, 2019 · 26 comments
Assignees
Milestone

Comments

@hwillson
Copy link
Member

Moving this here from the RA 3 PR thread: #2892 (comment)

cc @patroza


In latest (and previous) beta, it seems ssr: false queries are executed on server anyway, and appear to have a max processing time of around 5s, is that a new feature?

@hwillson hwillson added this to the Release 3.0 milestone Jun 17, 2019
@hwillson hwillson self-assigned this Jun 17, 2019
@patroza
Copy link

patroza commented Jun 20, 2019

The "max processing time of around 5s" part is incorrect, it was something else.

@hwillson
Copy link
Member Author

Thanks for reporting this @patroza. Unfortunately I haven't been able to re-create this (and #3197 shows that SSR disabling is working). I'll close this for now, but if there's any chance you can fire over a small runnable reproduction that shows this happening, I'll happily dig in further. Thanks again!

@patroza
Copy link

patroza commented Jul 6, 2019

hi @hwillson we are definitely running into this issue.
The only way I can make it stop, is by setting skip to true on the server.

On the server I will still see loading turn to true, and on another pass loading turn to false, but at least it seems it's not executing or waiting for any queries.

Can we try somehow the latest bits soon? e.g the tests are based on an unreleased @apollo/react-ssr package it seems.

I will try to see if I can come up with some kind of repro, but as always it's of course a complex project and setup..

@patroza
Copy link

patroza commented Jul 6, 2019

I have had success by treating ssr === false && this.context && this.context.renderPromises similar to the skip case.

Basically in the current release version, inside QueryData.execute:
instead of if (!skip) we would use if (!skip && (!this.context || !this.context.renderPromises || _a.ssr !== false))

I see that the current code is already different, but similar change should be applied.

There are probably more affected places, so imo: skip should throughout the queryData be equal to this.getOptions().skip || (this.context && this.context.renderPromises && this.getOptions().ssr === false)

so in short, right now I think ssr: false is perhaps not waiting on results, but still executes the queries. And if there are other queries processing which take longer, then the result of the query still ends up inside the memory cache of the ApolloClient, and gets put into the HTML document so that it is picked up by the frontend on initialization, leading to the client not needing to make the query.

On some level one might say; wow that's actually a great feature; you can eat your cake and still have it.
However the real problem occurs when the query doesn't finish before the request is finished; the load on the backend server is then double because both the server (futile) and client execute the query.

@hwillson
Copy link
Member Author

hwillson commented Jul 6, 2019

Thanks very much for the extra details @patroza - we'll get this fixed.

@hwillson hwillson reopened this Jul 6, 2019
@hwillson
Copy link
Member Author

@patroza Are you still seeing this happen with the latest beta's?

@patroza
Copy link

patroza commented Jul 19, 2019

@hwillson it looks like it's resolved, at least the client is doing the request, as the data is not coming in on the server side cache state anymore.

@hwillson
Copy link
Member Author

@patroza Great to hear, thanks!

@Pikachews
Copy link

Sorry to revive this thread, but I seem to be experiencing the same behavior on the latest version of Apollo (3.0.0).

I am using @apollo/react-hooks for making the query, specifically useQuery(...).
On the server, I am rendering with getMarkupFromTree (though I tried getDataFromTree, same behavior).
On the browser, with Javascript disabled (in order to prevent the client from making the request after the app has been mounted), upon loading a page that contains a useQuery(..., { ssr: false}), I can see that the query to the GraphQL server is made (based on logs from the express server where GraphQL is running).
With Javascript enabled, I see the same request being made twice on the GraphQL server, one from the SSR rendering and one from the browser. This effectively doubles the number of GraphQL requests made after upgrading from 2.x to 3.0.0. 🙁

For what it's worth, here are the rest of my apollo-related package versions:

    "@apollo/react-hooks": "^3.0.0",
    "@apollo/react-ssr": "^3.0.0",
    "apollo-cache-inmemory": "^1.6.3",
    "apollo-client": "^2.6.4",
    "apollo-link": "^1.2.12",
    "apollo-link-batch-http": "^1.2.12", (tried non-batch link too)
    "apollo-link-error": "^1.1.11",

My SSR ApolloClient:

  new ApolloClient({
    ssrMode: true,
    link: ApolloLink.from([
      new BatchHttpLink({ ... })
    ]),
    cache: new InMemoryCache({ ... })
  });

@hwillson
Copy link
Member Author

@Pikachews any chance you can provide a small runnable reproduction that shows this issue?

@Pikachews
Copy link

Sorry, didn't get a chance to create a repro, but it seems to be fixed with 3.0.1.

@Pikachews
Copy link

Hm, my mistake. It stopped due to a misconfiguration on my side. After fixing it, it still seems to be happening 😅 I'll try to get a repro up soon.

@OllieJennings
Copy link

OllieJennings commented Sep 3, 2019

@hwillson I am actually seeing this issue 3.0.1, the query gets executed, but the response does not get put into the cache (since SSR is false l presume), then it executes again on the client.

This is doubling all of our queries that have ssr: false, the ones without it, don't get recalled since their data is coming from the cache

@OllieJennings
Copy link

OllieJennings commented Sep 3, 2019

@hwillson interestingly if l use the BatchHttpLink then l do not see this issue of double requests, instead my queries with ssr: false do not call again on the client, they just use the cache from the server (as it seems like the BatchLink stores the results in cache regardless of the ssr status)

So the short version is, ssr: false queries are being called on the server regardless (if batched with another query), its just the BatchLink caches all queries

@OllieJennings
Copy link

@hwillson any chance we can get this re-opened?

@hwillson
Copy link
Member Author

hwillson commented Sep 5, 2019

@OllieJennings🤞#3435 should help with this.

@OllieJennings
Copy link

Ahh amazing @hwillson, when can we expect the latest version bump with this in?

@hwillson
Copy link
Member Author

hwillson commented Sep 6, 2019

@OllieJennings I've been holding off on publishing 3.1.0 until we have a clear answer for #3338. It looks like we might be okay there after all, and the remaining issues can be addressed (if necessary) in a point release. I'll finalize a few other small changes, and get 3.1.0 out this morning.

@OllieJennings
Copy link

@hwillson awesome, thanks a lot again. Will test the release tonight and report back

@OllieJennings
Copy link

Can confirm all is working @hwillson

@Blitss
Copy link

Blitss commented Sep 7, 2019

am I the only one who is still getting this issue? I'm using the latest version of @apollo/react-hooks like this:

info Direct dependencies
├─ @apollo/react-common@3.1.0
├─ @apollo/react-hooks@3.1.0
└─ @apollo/react-ssr@3.1.0
import * as ApolloReactHooks from '@apollo/react-hooks';

useQuery( /* query */ )({ ssr: false })

Also in my case along with updating to latest apollo, all subscriptions are being sent to the server which is weird. I temporarily solved this by using mock link which completes request without response if it's subscription.

UPD: Figured it out. It seems that after I call ReactDOM.renderToString again, this.context.renderPromises (

if (this.context && this.context.renderPromises) {
) becomes undefined which is why query get executed again. I should reuse result from getMarkupFromTree result.

@maapteh
Copy link
Contributor

maapteh commented Sep 21, 2019

By looking at the code and reviewing the metrics on my application i know this was not fixed, created a PR on #3515 which fixes this.

@neilff
Copy link

neilff commented Sep 27, 2019

We encountered this issue with our Apollo v3.0.1 + Next.js v7.0.2 app.

We primarily use the <Query> component. The solution that worked for us was to use skip={!isBrowser()} with isBrowser checking if we are in a Node environment or not. It seems to have resolved the issue for us.

FWIW, we tried upgrading to 3.1.1 but encountered issues with the upgrade, all queries were being sent to the client in a loading: true state, effectively breaking SSR.

@maapteh
Copy link
Contributor

maapteh commented Sep 28, 2019

@hwillson it looks like it's resolved, at least the client is doing the request, as the data is not coming in on the server side cache state anymore.

this statement is correct, but the call is still being made :( (#3515)

@OllieJennings
Copy link

@hwillson i've encounter this issue again. I have the following

<component> useQuery(...{ ssr: true })
  <nested /> useQuery(...{ ssr: false })
</component>

The nested components useQuery is being fully executed on the server still, is this a bug? or is this a restriction with useQuery?

@OllieJennings
Copy link

@hwillson just a thought? l using the apollo-link-batch-http lib, which could be the cause of this? will test when l can, but l suppose it could be grouping nested queries, and ignoring the ssr property?

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

No branches or pull requests

7 participants