ssr false queries still being called on server

[maapteh]

when using the new hooks implementation we saw double the load on the server. After debugging version 3.1.1 i found out that ssr: false components where also requesting our graphql endpoint while ssr treewalking. So this is a major bug.

ssr: false queries where still being executed on the server. Skip was doing it correctly so that gave me the correct information to debug further.

When i adjusted this it worked correctly. No unnecessary calls to my backend. It now respects the ssr setting on the useQuery hook and 'ssr: false' components are only requested client side.

It caused a lot of extra unneeded load to my backend when moving to hooks, the test case for it is not reflecting the actual behaviour.

closes #3500

[maapteh]

@hwillson or @benjamn hopefully you find some time to look at this one

[staylor]

this is a pretty major regression, please merge

[maapteh]

Dynatrace is showing with this fix its solved, i cant explain it with the current test because that one doesn't validate if the subscription is actually getting executed (which it did). Thanks for you feedback Staylor, for a moment i thought how can we be the only one :(

[hwillson]

Thanks for working on this @maapteh. Is there any chance you (or @staylor) can create a small runnable reproduction that demonstrates the problem this PR is fixing? Or add a test that shows the problem and how these changes fix it? As things stand, I'm having a hard time re-creating the issue. The test you modified (before your modifications) does show ssr: false working as intended, and we have other tests that verify this as well like:

react-apollo/packages/hoc/src/__tests__/ssr/getDataFromTree.test.tsx

Line 758 in c4c3b5d

it("shouldn't run queries if ssr is turned to off", () => {

It does sound like there's a bug here somewhere, but any extra details you can provide to help get a test(s) in place would be great. Thanks!

[maapteh]

I contacted you over spectrum one week ago (using there same name) because i have a runnable example but with a private key you need to add.

https://github.com/maapteh/graphql-modules-app this is a sample app which i use when i update to majors since it has most logic in the current real app. And both have the exact behaviour, and are also fixed with the above fix. I added a debug page with only one ssr false component and when i curl to it you see in the log the server resolves it by hitting my provider while this shouldn't happen at all. When i add my change it doesnt happen on curl and only happen when i request it clientside.

Your testcase is not a real testcase. It uses the mockedprovider which only responds when data is resolved for the render, but the actual scenario is that the call is being made while not returned and shouldnt be done in the first place :) I have no clue how to add a test for it since its happening in the private scope. In my opinion it's a real bug (which created a huge unnecessary load because also nothing happened with that data), which first need to be patched asap.

Im adding my change and the test still runs ok shows it doesn't validate that route...

0. checkout https://github.com/maapteh/graphql-modules-app
1. yarn
2. bash setup.sh
3. copy my given env for ./packages/server/.env (i passed private key over spectrum)
4. run yarn start
5. wait a little ...
6. goto http://localhost:4001/debug (ssr only component)

you will see this log twice:
https://github.com/maapteh/graphql-modules-app/blob/master/packages/server/src/modules/product/providers/product-data-loader.ts#L10

alos curl http://localhost:4001/debug gives a hit (while it shouldn't)

when changing code in ./packages/app/node_modules/@apollo/react-hooks/lib/react-hooks.cjs.js for my part in QueryData.prototype.startQuerySubscription,

restart it and happens once for the page, and not when doing a curl :)

[maapteh]

See the comment at your PR #3197

[hwillson]

Thanks @maapteh, this is very helpful.

[hwillson]

@maapteh I've re-created the issue using your reference app. This issue is happening because of an unnecessary/invalid extra render that's being triggered somewhere. See the following:

[REACT] [ event ] build page: /debug
[REACT] [ wait ]  compiling ...
[REACT] > Using external babel configuration
[REACT] > Location: "/Users/hwillson/Documents/apollo/2019-09/graphql-modules-app/packages/app/.babelrc"
[REACT] [ info ]  bundled successfully, waiting for typecheck results ...
[REACT] --- Render count 0
[REACT] Called getExecuteSsrResult
[REACT] renderPromises RenderPromises {
[REACT]   queryPromises: Map {},
[REACT]   queryInfoTrie:
[REACT]    Map {
[REACT]      { kind: 'Document', definitions: [Array], loc: [Object] } => Map { '{"id":"9200000113065845"}' => [Object] } } }
[REACT] --- Render count 1
[REACT] Called getExecuteSsrResult
[REACT] renderPromises undefined
[REACT] Called getExecuteResult
[ GRAPHQL] [GOOD] dataloader: https://[snip]/catalog/v4/products/9200000113065845?offers=cheapest&includeAttributes=false&format=json

Render count 0 above is the correct SSR render; on the server renderPromises is set, which is how the result is resolved and returned. But then for some reason a second unnecessary render is triggered, Render count 1, which no longer has the required renderPromises value in the context. renderPromises is how we tell that a request originated from the server side (which is set by getDataFromTree). Since the second render doesn't have renderPromises set, React Apollo skips its SSR result handling, and instead hits your API endpoint.

I'm still looking into why that extra render is being triggered. I'd like to figure out why that's happening instead of patching startQuerySubscription, as even with patching startQuerySubscription the extra render still occurs.

[maapteh]

Hi, i added yarn start:prod to make sure you have the frontend part running in production mode being build before starting it up. When switching back run yarn clean && yarn start

I now see the NextJS page is being rendered twice... wtf thats why the second cycle happens with empty renderPromises Isn't this because of the cache update?

[hwillson]

Isn't this because of the cache update?

@maapteh - if you switch your useGetProductQuery query to use fetchPolicy: "no-cache" like:

const { data } = useGetProductQuery({
  variables: { id },
  ssr,
  context,
  fetchPolicy: "no-cache"
});

you'll still see the double render. Since we're not writing anything to the cache with the above query, there shouldn't be any cache updates or renders caused by cache updates. I'm not super familiar with Next.js internals, but something is causing the WithApollo component in packages/app/src/graphql/apollo.js to render twice.

[maapteh]

I will read #3130 (comment) again and try to log my production app tomorrow. Thanks for reviewing Hugh, highly appreciated.

[hwillson]

@maapteh - I've confirmed the second render is being triggered by the Next.js integration. On the server side, 2 renders are happening because your demo app is calling WithApollo essentially twice. It's calling it the first time to render the results of the React Apollo getDataFromTree call, then it's calling it a second time to complete rendering for the initial server side request. So the second call has nothing to do with React Apollo - it's just a normal server side request where Next.js is responding by rendering the requested component.

As an example to verify this, if you adjust your WithApollo component to return a ssrComplete: true prop in getInitialProps like:

WithApollo.getInitialProps = async ctx => {
  ...
  return {
    ...pageProps,
    apolloState,
    ssrComplete: true
  }
}

then further adjust your WithApollo component to leverage this new prop like:

const WithApollo = ({ apolloClient, apolloState, ssrComplete, ...pageProps }) => {
  const client = apolloClient || initApolloClient(apolloState)
  return typeof window !== 'undefined' || (ssr && !ssrComplete)
    ? (
        <ApolloProvider client={client}>
          <PageComponent {...pageProps} />
        </ApolloProvider>
      )
    : null;
}

I think you'll find the extra render (and API hit) no longer happens on the server side.

I'll think about this a bit more and see if we can help address this on the React Apollo side of things, but in the meantime using some variation of the above will address the issue in your app.

[maapteh]

Today i had IE11 troubles so couldn't validate the changes in the production app i still have to make (prepare move to hooks) :(

-- removing comments, debugging further --

yes working perfect with the changes. Will investigate more hopefully tomorrow...

[hwillson]

Great, thanks for the update @maapteh!

I believe we can leverage the ssrMode option passed into the ApolloClient constructor, to get a failsafe integrated into React Apollo to prevent this from happening. I'm thinking of adjusting the QueryData.getExecuteSsrResult method like this:

private getExecuteSsrResult() {
  const treeRenderingInitiated = this.context && this.context.renderPromises;
  const ssrDisabled = this.getOptions().ssr === false;
  const fetchDisabled = this.refreshClient().client.disableNetworkFetches;

  const ssrLoading = {
    loading: true,
    networkStatus: NetworkStatus.loading,
    called: true,
    data: undefined
  } as QueryResult<TData, TVariables>;

  // If SSR has been explicitly disabled, and this function has been called
  // on the server side, return the default loading state.
  if (ssrDisabled && (treeRenderingInitiated || fetchDisabled)) {
    return ssrLoading;
  }

  let result;
  if (treeRenderingInitiated) {
    result =
      this.context.renderPromises!.addQueryPromise(
        this,
        this.getExecuteResult
      ) || ssrLoading;
  }

  return result;
}

When ssrMode is true, Apollo Client's disableNetworkFetches is true, meaning we want all network fetches to be disabled. Right now ssr: false doesn't take disableNetworkFetches into consideration, but with the above changes it would. So with these changes:

• If ssr: false is set and we're coming from a React SSR getDataFromTree or getMarkupFromTree call (treeRenderingInitiated is true), we return a default SSR loading state to prevent the running of any subsequent queries in the same render.
• If ssr: false is set and we've disabled network fetching (ssrMode is true), we return a default SSR loading state to prevent the running of any subsequent queries in the same render.

The above changes do help fix this issue, but I need to test the implications of these changes a bit further.

[maapteh]

@staylor did you try the above change in getExecuteSsrResult and does it fix it for you?

[hwillson]

Thanks very much for working on this @maapteh. I've made some adjustments, and am going to get this merged into master. I'll push out a new beta version, and let you know when it's ready. If you could run some tests with that version, that would be great. Thanks!

[mikebm]

so, this PR introduced an issue on client side rendering of network-only fetch policies when the query has ssr: false.

ssrForceFetchDelay doesn't work well, and so a lot of people in their client code do the following (myself including):

apolloClient.disableNetworkFetches = true;

then in their hydrate callback, they do:

apolloClient.disableNetworkFetches = false;

Unfortunately, this change triggers the above stated query setup to never fire on the client after a SSR.

I am wondering if this code change should go from:

if (ssrDisabled && (treeRenderingInitiated || fetchDisabled)) {

to:

if (treeRenderingInitiated && (ssrDisabled || fetchDisabled)) {

This would make it so fetchDisabled is only utilized on the server and not client, which is what the code I added here was originally only supposed to run on.

Thoughts @hwillson ?

[Axxxx0n]

@mikebm This is also an issue when using ssrForceFetchDelay