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

different server-side rendering behaviour between v2 and v3 #3338

Closed
cusxio opened this issue Aug 10, 2019 · 47 comments
Closed

different server-side rendering behaviour between v2 and v3 #3338

cusxio opened this issue Aug 10, 2019 · 47 comments
Assignees

Comments

@cusxio
Copy link

cusxio commented Aug 10, 2019

Current Behaviour

v2 and v3 of react-apollo have vastly different behaviour/timings server side.

On v3, it seems that react-apollo has an additional render pass/intermediate state on the server side that breaks v2 behaviour.

The additional render pass seems to output the following:

const ssrLoading = {
loading: true,
networkStatus: NetworkStatus.loading,
called: true,
data: {}
};

Reproduction:

v2: https://codesandbox.io/s/with-apollo-xrj2c

v3: https://codesandbox.io/s/with-apollo-8luew

Check out the Terminal output difference of both v2 and v3,

v2 only has two render pass, but v3 has three render pass.

The additional render pass in v3 currently breaks our app, as we rely on the variable data being there.

Expected Behaviour

v3 server side rendering behaviour should be similar to v2

@hwillson hwillson self-assigned this Aug 11, 2019
@NinjaOnRails
Copy link

NinjaOnRails commented Aug 11, 2019

I'm having a similar issue here
The last part is relevant:

Specific observations using next@9.0.3 and { getDataFromTree } from '@apollo/react-ssr' (@apollo/react-ssr@3.0.0):

react-apollo@3.0.0: SSR works, cookies don't get passed, no error
react-apollo@2.5.8: SSR breaks, cookies get passed, no error (Tested on Chrome and Firefox. On Safari, cookies aren't even saved)

EDIT:
I've fixed it for myself using the latest libraries next@9.0.3, next-with-apollo@4.2.0 and react-apollo@3.0.0. Cookies are passed with every request, SSR working as expected and no errors. Code is here

@KittyGiraudel
Copy link

I can confirm. react-apollo@^3.0.0 has a rendering pass on the server with loading being true.

@youfoundron
Copy link

Also seeing this issue. Updating Next did not fix this for me.

@flippidippi
Copy link

We are seeing the same issue of the data on SSR still showing loading: true

@mikebm
Copy link
Contributor

mikebm commented Aug 28, 2019

Also experiencing this. The SSR is only returning our loading components.

@mikebm
Copy link
Contributor

mikebm commented Aug 29, 2019

I've been debugging this a little bit today and it appears to be isolated to network-only fetchPolicy. Still unsure why and will keep digging, but one could potentially use cache-and-network as workaround. I personally went with network-only because I was worried about the client side cache getting too big for my particular case since there is currently no TTL support.

@mikebm
Copy link
Contributor

mikebm commented Aug 29, 2019

@hwillson I was able to "resolve" my issue where a fetchPolicy of network-only causes only the loading component to render during SSR. It appears to be due to the initial observable getting initialized in execute which is before fetchData is called. I was able to adjust the updateObservableQuery function with the below code:

if (this.context.renderPromises && 
    (newObservableQueryOptions.fetchPolicy === 'network-only' || 
     newObservableQueryOptions.fetchPolicy === 'cache-and-network')) {
	newObservableQueryOptions.fetchPolicy = 'cache-first';
}

This adjusts the options to be consistent with what fetchData does. Ideally, I would think this logic would live in initializeObservableQuery, but the code in updateObservableQuery would immediately blow away the options.

I'd also like to point out that getOptions is pretty inefficient as it always returns a new object and is called all over the place.

I am not sure this addresses the original posters complaint of an additional "loading" render during SSR, but how it works now makes it more consistent with how it works on the client. The main issue I can see is that network-only fetches with useQuery results in no data returned and instead whatever component you have rendering in your loading event which some form of my above findings would resolve.

@hwillson
Copy link
Member

Hi all - question regarding the OP; would people prefer to see the initial loading state of true when using SSR, or not? React Apollo 2 skipped displaying it (thereby skipping the additional render), but we have had requests to enable it (hence the reason why it's in RA 3). I see now that this wasn't mentioned in the breaking changes (argh 😳), so I can revert things back to the way they were with RA 2; that is, no initial loading: true when using SSR. Having the additional loading: true step does add some extra flexibility though, so I'd love to hear what everyone prefers before (possibly) changing this back.

Thanks for looking into this @mikebm! I don't think your change will resolve the initially reported issue, but your suggested change makes a lot of sense regardless. Any chance you'd be interested in submitting that as a PR?

@mikebm
Copy link
Contributor

mikebm commented Aug 30, 2019

I personally think the loading state should stay. It makes things consistent. In my PR, which I just opened, also addresses an issue where disabling SSR on a query was still resulting in the query being fired but the result ignored. In this case, I had to still return a state when the hook was executed, so I chose to use the ssrLoading event.

@cusxio
Copy link
Author

cusxio commented Aug 31, 2019

Having the additional loading: true step does add some extra flexibility though, so I'd love to hear what everyone prefers before (possibly) changing this back.

Is there an example of a use case where loading: true is desired?


Currently in our app, because loading: true during the initial render, and we fail to account for it, our app does a redirection because it can't find the needed data (i.e. currentUser).


Question:

Correct me if I'm wrong, but is this how server-side rendering work in Apollo?:

  1. Initial render, recursively finds all the Query components in the app via calling renderToString.
  2. Wait for all promises to resolve to get the data.
  3. Call renderToString again with the data to get the correct markup.

@KittyGiraudel
Copy link

Hello there. 👋

We have been using Apollo for over 2 years at N26 and have been delighted with it so far. We were stoked to update to v3 and migrate to hooks for more elegant components. However… this breaking change is quite a bummer in my opinion. :(

I might be missing something, but I genuinely don’t understand the benefit of shipping a loading state to the client. If the server renders with loading: true, that means it will generate markup for the loading state of all querying components. Correct?

The point of SSR — at least the way we use SSR — is that the page should be readable/usable when hitting the client, so much that we could skip shipping the bundles entirely and have everything there. This change makes that impossible as displaying loaders is not quite achieving that.

It would be lovely if we could restore the v2 behaviour somehow. 😊 ✨

@flippidippi
Copy link

I completely agree with @hugogiraudel and @cusxio. I think the previous default makes more sense from an SSR "ready for client" perspective. We rely on data being ready and not loading, like currentUser. I don't see the point of loading = true if we are using getDataFromTree in SSR before returning the SSR pages to the client.

I'm not sure what the use case is for loading = true, but it seems like it at least shouldn't be the default. If there is a use for it, maybe have it as a feature flag in Apollo init.

❤️

@mikebm
Copy link
Contributor

mikebm commented Aug 31, 2019

I think the real question is why are your components stuck in a loading state? Ideally your component would never render to the client the loading component during a SSR, except maybe when SSR is off for the query. I feel like you all might be running into a different bug that prevented the query from finishing and either giving an error or data. What fetchPolicy are you guy's using?

@OllieJennings
Copy link

@hwillson It seems like Cookies are not being sent when on SSR unless you manually include req.headers (this isn't required on client, just server)

@mikebm
Copy link
Contributor

mikebm commented Sep 3, 2019

@hwillson It seems like Cookies are not being sent when on SSR unless you manually include req.headers (this isn't required on client, just server)

That's normal. You have to do it manually. On each request, when you build your Apollo Client on the server, you need to pass headers from your request object (req.headers, in the case of express) to createHttpLink. The apollo client doesn't know about express's request nor should it.

@OllieJennings
Copy link

@mikebm got ya, cheers for the explanation

@flippidippi
Copy link

The components aren’t stuck in loading state, they just begin in loading state then switch to done after it gets the data from cache. But we would like SSR to skip the loading state completely as we already have all the data loaded. It happens with the default fetch policy.

To confirm we aren’t having the cookies issue as we already manually attach this to the request.

@mikebm
Copy link
Contributor

mikebm commented Sep 3, 2019

The components aren’t stuck in loading state, they just begin in loading state then switch to done after it gets the data from cache. But we would like SSR to skip the loading state completely as we already have all the data loaded. It happens with the default fetch policy.

To confirm we aren’t having the cookies issue as we already manually attach this to the request.

So, you are saying the SSR is only returning the loading state to your component and never loading: false, with data populated? In this scenario, you are also saying that your apollo cache was populated with the data. If this is true, then it sounds more like a bug with your component not getting re-rendered with data.

I assume you are doing the SSR via something like so?

const html = await renderToStringWithData(serverApp);

I was definitely not experiencing this with the default cache policy, which is cache-first. It would go through the loading state, but once the data was resolved, my component was re-rendered and loading set to false and the final result of the SSR contained my component in a non-loading state.

@flippidippi
Copy link

flippidippi commented Sep 4, 2019

We are using getDataFromTree and passing a Apollo client with initial cached data to it. It’s not that it’s not switching to loading: false, because it is, but they way we wrote our code and what we expect from SSR, is that the components that already have their data from SSR should start off as loading: false. Otherwise we get a flash of loading on all components and stuff like currentUser that we know we already have in cache we would now have to wrap in waiting to finish loading functions.

I don’t have time right now but maybe I can try to create a small repo of how we are doing it and make sure it’s not something we are doing wrong. It does seem like others are describing our exact issue.

@hwillson
Copy link
Member

hwillson commented Sep 5, 2019

I'd really like to get this issue resolved before publishing 3.1.0, so let's all hash out a plan of attack together.

Background

In React Apollo 2, the Query component returns null for the first render triggered during SSR (when the query is loading on the server). So when a component like the following

const SomeComponent = () => {
  return (
    <Query query={CAR_QUERY}>
      {({ loading, data }) => {
        if (!loading) {
          const { make, model, vin } = data.cars[0];
          return (
            <div>
              {make}, {model}, {vin}
            </div>
          );
        }
      }}
    </Query>
  )
};

is rendered on the server, its first pass essentially renders a component that looks like:

const SomeComponent = () => null;

One advantage render prop functions have over hooks in this situation is that they encapsulate all query result handling, and can then decide if they should or should not return null to the parent component. In other words, if the returned query result is null because we're handling the first step of SSR, the following destructuring won't break:

{({ loading, data }) => {

This is because internally before running the render prop function with the results of the initial SSR query call (which we haven't received yet from Apollo Client), we just return null instead of triggering the render prop function.

When it comes to the React Apollo 3 hooks approach however, useQuery calls generally look like this:

const SomeComponent = () => {
  const { loading, data } = useQuery(CAR_QUERY);
  if (!loading) {
    const { make, model, vin } = data.cars[0];
    return (
      <div>
        {make}, {model}, {vin}
      </div>
    );
  }
  return null;
};

Consumers of useQuery are deciding how to handle destructuring, and this isn't something React Apollo can control (unlike the render prop function approach). During the first SSR pass, since we can't hold up a render, we have to return something. The render prop function approach of returning null doesn't work quite as nicely, since most people aren't expecting a null result from useQuery, and their destructuring will fail. To workaround this, React Apollo 3 currently returns a loading: true based result for the first SSR pass, which is accurately mirroring what's really happening behind the scenes. Unfortunately, as mentioned in several comments in this thread, it can lead to un-necessary loading: true states being shipped to the client.

Solution

It is possible to add extra logic to block loading: true results from being sent to the client, but that extra work can be a pain (especially since this wasn't necessary in React Apollo 2).

Looking ahead for ways to change this, would people prefer that we drop the initial SSR loading: true result, and instead return null from useQuery as the query is loading? To simulate how the Query comonent works in React Apollo 2, you could then update your component like:

const SomeComponent = () => {
  const result = useQuery(CAR_QUERY);
  if (result) {
    const { loading, data } = result;
    if (!loading) {
      const { make, model, vin } = data.cars[0];
      return (
        <div>
          {make}, {model}, {vin}
        </div>
      );
    }
  }
  return null;
};

If people don't care about SSR, they don't have to worry about checking for null, since non-SSR results can never be null. Would something like this be helpful?

@mikebm
Copy link
Contributor

mikebm commented Sep 5, 2019

I'd still like to narrow down the "it can lead to un-necessary loading: true states being shipped to the client." as this implies the SSR didn't wait for the query to resolve and return data or an error which would have caused the useQuery to trigger a re-render with a more completed state.

@hwillson
Copy link
Member

hwillson commented Sep 5, 2019

@mikebm When useQuery is fired on the server, it has to complete the initial render. As part of that initial render, it preps and fires the query, and gets ready to receive the result. While it's waiting for the result, it has to return something to complete the render. React Apollo 2 is returning null, React Apollo 3 is return a result object with loading: true. We can't hold up returning that initial result, and if people are shipping that initial result to the client, it gets out.

@mikebm
Copy link
Contributor

mikebm commented Sep 5, 2019

@mikebm When useQuery is fired on the server, it has to complete the initial render. As part of that initial render, it preps and fires the query, and gets ready to receive the result. While it's waiting for the result, it has to return something to complete the render. React Apollo 2 is returning null, React Apollo 3 is return a result object with loading: true. We can't hold up returning that initial result, and if people are shipping that initial result to the client, it gets out.

I understand this, but the server shouldn't be returning those initial results to the client because the SSR will wait for all the renderPromises to resolve, which once they do, it will re-trigger the components to re-render in a non-loading state. So, my question is, aside from the bug I recently fixed, how is it that these components are being returned to the client in a loading state when using the proper SSR features in this library such as getDataFromTree/getMarkupFromTree/renderToStringWithData. getDataFromTree would require rehydration on the server when rendering the markup though.

@hwillson
Copy link
Member

hwillson commented Sep 5, 2019

@mikebm For sure, and great question. The original reproduction links in this issue thread show the loading: true differences between React Apollo 2 and 3, but they don't really show an actual issue. The content shipped to the client is still valid and its loading state is false. The differences between RA 2 and 3 can be explained (#3338 (comment)), but ultimately the loading: true state shouldn't be trickling out to the client.

So ... I guess we need more details on what the actual issue is there then. Is anyone here able to put together a reproduction that shows how the additional loading state is causing grief? Please note that if you want to use the next.js with-apollo demo app as a starting point, they just released a new version yesterday that is using @apollo/react-hooks and @apollo/react-ssr.

@kedarguy
Copy link

kedarguy commented Sep 8, 2019

@mikebm For sure, and great question. The original reproduction links in this issue thread show the loading: true differences between React Apollo 2 and 3, but they don't really show an actual issue. The content shipped to the client is still valid and its loading state is false. The differences between RA 2 and 3 can be explained (#3338 (comment)), but ultimately the loading: true state shouldn't be trickling out to the client.

So ... I guess we need more details on what the actual issue is there then. Is anyone here able to put together a reproduction that shows how the additional loading state is causing grief? Please note that if you want to use the next.js with-apollo demo app as a starting point, they just released a new version yesterday that is using @apollo/react-hooks and @apollo/react-ssr.

I opened #3390 that shows the error we get in getDataFromTree because of this issue.

I now updated the repo further.

Here is the github repo

And link to code sandbox

You can now see that on the initial render we get the loading component.

I found that it happens when we query for client data with @client directive, and then in a nested component we query for remote data.

@lifeiscontent
Copy link

lifeiscontent commented Sep 11, 2019

this might be related: #3486

also @flipxfx you can use apollo with _app.js the reason why the new example is using page components only is due to how the new Next.js 9 automatic prerendering works. If you wrap _app.js you automatically opt-out of it on every page.

@flippidippi
Copy link

Yes but we are fine with opting out on every page, as it’s an internal site that requires Apollo on every page. But for some reason I can’t get it working correctly via the _app.js.

@lifeiscontent
Copy link

ping @hwillson please take a look at #3338 (comment) seems like there are some issues related / around it.

@lasseborly
Copy link

I found that it happens when we query for client data with @client directive, and then in a nested component we query for remote data.

I can reproduce the same behavior @kedarguy.

@carlows
Copy link
Contributor

carlows commented Sep 19, 2019

Btw this is also happening to me when using the @client directive but from v2, I don't think this is related to v3 changes specifically

@mariohoyos92
Copy link

I agree with the sentiment that when I use SSR I am expecting the page to come with all queries resolved already. I'm sure a ton of us use SSR for SEO purposes and it doesn't do us much good in that regard to show an empty page while we fetch data. The advantage to SSR is handling this on the server and delivering a fleshed out page, otherwise might as well just run with create-react-app and not worry about SSR.

@kaanozcan
Copy link

kaanozcan commented Sep 30, 2019

If renderToStringWithData worked like an iterator and yield each render one by one we could do pre-rendering. Contract would be changed alot but we could send responses as fast as possible to the client and the people who doesn't want to do pre-rendering would just send the last rendered markup. It also would make sense that loading was true at some point. #3545

@renato
Copy link

renato commented Nov 15, 2019

I'm facing the same issue when updating from 2.5.8 to 3.1.3. So if I need the initial server render to resolve all queries for SEO purposes, there's no workaround, the only solution currently is sticking to v2?

@NinjaOnRails
Copy link

I'm facing the same issue when updating from 2.5.8 to 3.1.3. So if I need the initial server render to resolve all queries for SEO purposes, there's no workaround, the only solution currently is sticking to v2?

I use it with Next JS and the temporary fix is to call the queries and pass them to the component with getInitialProps. This is how I get the data for SSR

@flippidippi
Copy link

I was able to resolve the initial SSR issue, it now comes back as loaded, but we are still having an issue when changing routes and the "SSR" not working before changing routes.

Example repo: https://github.com/flippidippi/next-apollo-app

@bdew
Copy link

bdew commented Dec 2, 2019

To workaround this, React Apollo 3 currently returns a loading: true based result for the first SSR pass, which is accurately mirroring what's really happening behind the scenes. Unfortunately, as mentioned in several comments in this thread, it can lead to un-necessary loading: true states being shipped to the client.

I think there's more going on here, or two different bugs.

Those loading: true results were working fine with hooks in apollo client 2, the server would wait for queries to resolve then rerender the components to show the normal (not loading) state before it was sent to the browser.

With apollo client 3 getDataFromTree promise seems to resolve right away without waiting for queries to resolve, so the markup is sent with components in loading state.

I don't see how returning null instead of {loading: true} fix that or what it would add other than more boilerplate in components.

@EyMaddis
Copy link

EyMaddis commented Dec 2, 2019

In my setup the backend seems to load everything just fine, meaning that the apollo cache state is always filled with the correct cache state. However, in reality, the serverside rendered result is showing the loading state. The client-side always renders the non-loading state because the cache is prefilled from the DOM.

I logged the loading states (on the server) and it is: true, false, true which is super weird. The first render is gathering the queries, the second one is the response, but the third is the Next.js rendering with a prefilled cache.
Right now I could only see this in development on my machine. Once it is that way every request is as such. I have to restart the server to fix it. I could not yet see this in production, but this is also just a staging environment.

I would love to share a reproducible repo but did not find a proper way to reliably reproduce this issue.
My current guess is that hot reload might mess stuff up after saving a couple of files.

I also debugged deep into apollo and could not see why loading is returned, because the cache was also valid for the SSR query handling (checked via the debugger).

@lifeiscontent
Copy link

lifeiscontent commented Dec 3, 2019

@EyMaddis what is your defaultFetchPolicy / fetchPolicy? I found that cache-and-network did what you were saying, but cache-first worked as expected, with the exception that if you don't update the cached objects via a mutations update function then you'll get stale data. In my own case, we had a viewed matches term which wasn't updating in the cache-first case due to it changing outside of Apollo (legacy code).

@carlosbaraza
Copy link

Coming from #3390

I think the behavior of @client is quite confusing. I am trying to do something similar to what is showcased in the docs.

During SSR using Next and getDataFromTree:

  1. First, it fetches the user and populates the cache using writeData.
  2. When I run a @client query for the user data, it returns a loading: true, data: {}, instead of the user data it should return.

My workaround is reading the cache directly using readQuery, which seems to return the data without that loading: true step.

@EyMaddis
Copy link

@lifeiscontent I tried multiple strategies, but they do not provide any change. I also made sure (using cache-only) that the default settings are applied (because the app does not load anything then).

@lifeiscontent
Copy link

@EyMaddis Can you provide a minimal reproducible use case?

@EyMaddis
Copy link

@lifeiscontent I would love to, unfortunately this only happens seemingly at random while developing. My current guess is that the auto-reload while saving files (in a Next.js project) might break this after a lot of edits.
It might also be a memory issue (+ a leak) that can lead to this, but I tried to play around with max memory limits without any noticeable change.

@krzksz
Copy link

krzksz commented Jan 1, 2020

Hey, I've just stumbled across this bug and it seems like reusing local state variables inside queries using @export(as: "variableName"), here's a reproduction where I'm extracting skip variable from the cache:
https://codesandbox.io/s/with-apollo-r3599?fontsize=14&hidenavigation=1&theme=dark

@hwillson hwillson self-assigned this Jan 7, 2020
@hwillson
Copy link
Member

hwillson commented Jan 8, 2020

This issue thread has gone in a few different directions which makes it difficult to extract (and track) actionable items. I'll reply to some of the oustanding comments below (the ones that I think encompass the bulk of the questions), but to anyone still having an issue with this, please re-read my comments in #3338 (comment) to understand why React Apollo v2 and v3 handle SSR differently.

@seantimm Regarding #3338 (comment), I'd love to hear more. Would you be able to open this in a new issue, possibly with a reproduction?

@kedarguy Regarding #3338 (comment), first off thanks for the reproduction. What this reproduction shows however is exactly what I've explained in #3338 (comment). There is an additional loading state happening on the server side, that needs to be handled in your code. You can block the extra loading state from being rendered (which is ultimately leading to the destructuring error you're seeing. There are different ways to address this, but one quick way is to 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 to know when to render your component tree:

const WithApollo = ({ apolloClient, apolloState, ssrComplete, ...pageProps }) => {
  const client = useMemo(
    () => apolloClient || initApolloClient(apolloState),
    []
  );

  if (!ssrComplete) {
    return null;
  }

  return (
    <ApolloProvider client={client}>
      <PageComponent {...pageProps} />
    </ApolloProvider>
  );
}

As mentioned in #3338 (comment) I'm all ears if people would like a different solution implemented, but as things stand that initial loading state is required.

@bdew Regarding #3338 (comment), the loading true results were working with AC 2 because of the reasons I outlined in #3338 (comment) (in the Background section). The suggested workaround I mentioned was just an idea, and I'm open to other suggestions here. This isn't a bug though. The issue is caused by architecture differences between v2 and v3, due to the transition from using render props for everything, to hooks.

@krzksz Regarding #3338 (comment), I updated your repro to use version 3.1.3 of @apollo/react-hooks and @apollo/react-ssr, and it appears to work properly. If it doesn't, please open this in a new issue.

Given that this issue thread has grown a bit unwiedly, I'll close it off. Please open new issues for further SSR related problems. Thanks!

@debugpai
Copy link

debugpai commented Jul 13, 2020

@hwillson I was facing problem with this issue as well. But I managed to get the final server side rendered state to show the results by just using the rendered string from getMarkupFromTree instead of calling renderToString as mentioned in the docs.

import { getMarkupFromTree } from '@apollo/client/react/ssr';
import { renderToString } from 'react-dom/server';
 
const html = await getMarkupFromTree({
  tree: App,
  context: {},
  renderFunction: renderToString
});

instead of

await getDataFromTree(App);
const html = renderToString(App);

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

No branches or pull requests