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

v3 - SSR - Queries render with loading: true even when data exists (Example repo provided) #6631

Open
bretthadley opened this issue Jul 17, 2020 · 12 comments

Comments

@bretthadley
Copy link

bretthadley commented Jul 17, 2020

Intended outcome:
Using apollo getDataFromTree I expect the html produced by renderToString to not be in loading state.

Actual outcome:
The query loading is true even when data is populated. So the component will hit the

if (loading) return <div>Loading</div>

How to reproduce the issue:

Here is a repo. You can see on the server renderToString() the loading value is true even though data is populated with the query result. You can also see window.__APOLLO_STATE__ is correctly populated with the result.

https://github.com/bretthadley/apollo-issue-demo

Versions

System:
    OS: macOS 10.15.5
  Binaries:
    Node: 14.3.0 - /usr/local/bin/node
    Yarn: 1.17.3 - ~/.yarn/bin/yarn
    npm: 6.14.5 - /usr/local/bin/npm
  Browsers:
    Chrome: 83.0.4103.116
    Firefox: 68.0.2
    Safari: 13.1.1
  npmPackages:
    @apollo/client: ^3.0.2 => 3.0.2
@benjamn
Copy link
Member

benjamn commented Jul 17, 2020

Not entirely sure if renderToString can be made to work correctly here, but you don't need it, because getDataFromTree returns a Promise<string> that gives you the markup:

diff --git a/src/server.js b/src/server.js
index 34f6301..a8f9ab4 100644
--- a/src/server.js
+++ b/src/server.js
@@ -25,9 +25,7 @@ server
       </ApolloProvider>
     );
 
-    getDataFromTree(app)
-    .then(() => {
-      const markup = renderToString(app);
+    getDataFromTree(app).then(markup => {
       const initialState = apolloClient.extract();
 
         if (context.url) {

Thanks to your reproduction, I can confirm that this renders the following HTML, which seems correct/better?

<!doctype html>
    <html lang="">
    <head>
        <meta http-equiv="X-UA-Compatible" content="IE=edge" />
        <meta charset="utf-8" />
        <title>Welcome to Razzle</title>
        <meta name="viewport" content="width=device-width, initial-scale=1">
        <script src="http://localhost:3001/static/js/bundle.js" defer crossorigin></script>
    </head>
    <body>
        <div id="root"><section><ul><li><div><span>1. </span><a href="http://nextjs.org">next</a></div></li><li><div><span>2. </span><a href="https://kesla">akhil</a></div></li><li><div><span>3. </span><a href="https://drgf">iuhg</a></div></li><li><div><span>4. </span><a href="https://with-apollo-subscription.now.sh/subscription">Hzz</a></div></li><li><div><span>5. </span><a href="http://google.com">Google</a></div></li><li><div><span>6. </span><a href="https://kevinrodriguez.io">TEST</a></div></li><li><div><span>7. </span><a href="https://band.us/ko">5555</a></div></li><li><div><span>8. </span><a href="https://band.us/ko">22</a></div></li><li><div><span>9. </span><a href="https://next-with-apollo.now.sh/">hiya gofod</a></div></li><li><div><span>10. </span><a href="http://seminor/">b、ん、m</a></div></li></ul></section></div>
        <script>
          window.__APOLLO_STATE__={"Post:ckcq6o3as0f2901558vrifyee":{"id":"ckcq6o3as0f2901558vrifyee","__typename":"Post","title":"next","votes":0,"url":"http://nextjs.org","createdAt":"2020-07-17T12:13:12.000Z"},"Post:ckcpx0huv0mzp0198uhppe5dq":{"id":"ckcpx0huv0mzp0198uhppe5dq","__typename":"Post","title":"akhil","votes":14,"url":"https://kesla","createdAt":"2020-07-17T07:42:54.000Z"},"Post:ckcpwzq4z0kio0130u2e42dy7":{"id":"ckcpwzq4z0kio0130u2e42dy7","__typename":"Post","title":"iuhg","votes":3,"url":"https://drgf","createdAt":"2020-07-17T07:42:18.000Z"},"Post:ckcpvx1xa0hb20145pd3rdmgg":{"id":"ckcpvx1xa0hb20145pd3rdmgg","__typename":"Post","title":"Hzz","votes":0,"url":"https://with-apollo-subscription.now.sh/subscription","createdAt":"2020-07-17T07:12:14.000Z"},"Post:ckcprlkbq0gnj01651r868ahx":{"id":"ckcprlkbq0gnj01651r868ahx","__typename":"Post","title":"Google","votes":10,"url":"http://google.com","createdAt":"2020-07-17T05:11:20.000Z"},"Post:ckcppcgx708pz0105k2f67bbl":{"id":"ckcppcgx708pz0105k2f67bbl","__typename":"Post","title":"TEST","votes":26,"url":"https://kevinrodriguez.io","createdAt":"2020-07-17T04:08:16.000Z"},"Post:ckcpo813504ov0154l2wlnkqg":{"id":"ckcpo813504ov0154l2wlnkqg","__typename":"Post","title":"5555","votes":6,"url":"https://band.us/ko","createdAt":"2020-07-17T03:36:49.000Z"},"Post:ckcpo7pr604o80154qtob8tqi":{"id":"ckcpo7pr604o80154qtob8tqi","__typename":"Post","title":"22","votes":4,"url":"https://band.us/ko","createdAt":"2020-07-17T03:36:35.000Z"},"Post:ckcpmz31m00hg0105mcazgdco":{"id":"ckcpmz31m00hg0105mcazgdco","__typename":"Post","title":"hiya gofod","votes":15,"url":"https://next-with-apollo.now.sh/","createdAt":"2020-07-17T03:01:52.000Z"},"Post:ckcplo0fs02yb010322i0alzf":{"id":"ckcplo0fs02yb010322i0alzf","__typename":"Post","title":"b、ん、m","votes":12,"url":"http://seminor/","createdAt":"2020-07-17T02:25:16.000Z"},"ROOT_QUERY":{"__typename":"Query","allPosts({\"first\":10,\"orderBy\":\"createdAt_DESC\",\"skip\":0})":[{"__ref":"Post:ckcq6o3as0f2901558vrifyee"},{"__ref":"Post:ckcpx0huv0mzp0198uhppe5dq"},{"__ref":"Post:ckcpwzq4z0kio0130u2e42dy7"},{"__ref":"Post:ckcpvx1xa0hb20145pd3rdmgg"},{"__ref":"Post:ckcprlkbq0gnj01651r868ahx"},{"__ref":"Post:ckcppcgx708pz0105k2f67bbl"},{"__ref":"Post:ckcpo813504ov0154l2wlnkqg"},{"__ref":"Post:ckcpo7pr604o80154qtob8tqi"},{"__ref":"Post:ckcpmz31m00hg0105mcazgdco"},{"__ref":"Post:ckcplo0fs02yb010322i0alzf"}],"_allPostsMeta":{"__typename":"_QueryMeta","count":18544}}};
        </script>
    </body>
</html>

@bretthadley
Copy link
Author

bretthadley commented Jul 17, 2020

Thanks for the quick reply. Prior to my upgrade to v3 yesterday this worked on the latest v2 version as expected. Unfortunately due my usage of renderToNodeStream & emotion css steaming it means I can't use that returned string (it's also not mentioned in the documentation on v3 or v2.6)

Is there now any purpose to the renderToStringWithData vs getMarkupFromTree vs getDataFromTree?

@benjamn
Copy link
Member

benjamn commented Jul 17, 2020

Can you share an example of the renderToNodeStream-style code?

I'm interested in having an SSR function that supports streaming. The last time we investigated this, it seemed like Suspense might end up solving the problem, but I'm less convinced that's what people really want/need, these days.

Is there now any purpose to the renderToStringWithData vs getMarkupFromTree vs getDataFromTree?

Since the other two functions just call getMarkupFromTree now, I would say the differences between them are not very important anymore.

@bretthadley
Copy link
Author

Sure, here is the same example but adding renderToNodeStream & emotion css streaming with some random styled boxes.

https://github.com/bretthadley/apollo-issue-demo/tree/streaming

@jimrandomh
Copy link

There appear to be two related issues here, one server side and one client side. The server-side issue is renderToString not working; for our project (LessWrong), this made the SSR html blank. Using the return value from getDataFromTree works, and looks like the right solution, so this is mostly a documentation issue; the migration guide should say that renderToString is no longer suitable. Streaming may still be an issue, but we weren't doing that before.

But there's a second issue, which is client side. The first call to useQuery for a given query, with ssr: true, returns the correct data, but with loading: true. The cache is correctly pre-populated and no network request is sent, and it immediately does an update/rerender with loading:false, but all the extra rerendering has a noticeable performance imact.

@benjamn
Copy link
Member

benjamn commented Jul 20, 2020

@jimrandomh Can you open a new issue for the second problem? No need for a long description. Feel free to copy/paste or link back to your comment here.

@jimrandomh
Copy link

Looks like maapteh beat me to it (#6651). I confirm that maapteh's issue is the same as the one I referred to.

@MikeAliG
Copy link

Hello everyone! Any updates on getDataFromTree? I have the same problem as @jimrandomh . SSR doesn't wait for data and returns <Loading /> component.

@Superoryco
Copy link

@AliTheAli In order to may SSR work again, may be just change the render logic like so

if (data) {
return <div>data</div>;
} else if (loading) {
return <div>data</div>;
}

@gustavosabonare
Copy link

gustavosabonare commented Aug 1, 2020

In my case, I was using axios as fetcher, changing to cross-fetch fixed the problem for me.

@michaelchiche
Copy link

I had a similar problem, which i tried to solve using #6631 (comment), but it did not work out for me.

I then saw in the logs of my server that I had problem with the cache, not willing to automatically merge a field.
I fixed that, and the loading was then correct.

Hope this can help other people having this issue

@DouglasGabr
Copy link

I'm having the same issue, using Next.js, I noticed it happens if I use any fetchPolicy that forces network requests (cache-and-network in my case).
The following happens:

  1. query returns loading: true
  2. immediately re-renders with loading: false, as expected
  3. for some reason, it re-renders (probably because of Next.js render to string), and now loading: true is back, even if the query have results.

That doesn't happen if fetchPolicy is cache-first.
I've tried to use nextFetchPolicy: 'cache-first', but it doesn't solve the problem.

So my workaround was to simply put ssr: false in my query since it was not important on the server, but I think this is a bug and should be fixed.

I'll try to include a repo with the scenario I'm describing as soon as possible.

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

No branches or pull requests

8 participants