Improve with-apollo example to more closely align to Next.js standards

[leerob]

What example does this report relate to?

with-apollo

What version of Next.js are you using?

latest

What version of Node.js are you using?

latest

What browser are you using?

any

What operating system are you using?

any

How are you deploying your application?

any

Describe the Bug

The current with-apollo example fetches data and adds it to the cache, but never returns that data inside getStaticPaths / getSeverSideProps.

await apolloClient.query({
  query: ALL_POSTS_QUERY,
  variables: allPostsQueryVars
})

Expected Behavior

All other usage of getStaticProps / getServerSideProps show forwarded the data to the component via props (hence the names). This seems really confusing to me. I think we should use props here instead.

To Reproduce

View the with-apollo example

[leerob]

Follow up from this PR: #21956

[filipesmedeiros]

Hey @leerob I think it actually does. Inside addApolloState, we extract the server-side ApolloClient cache and pass it down to the client. I agree with the sentiment that this feels confusing and unnecessary. I think Apollo is big enough that Next should have an opinionated way/example of dealing with it (both for SSR and Client rendering). See my comments here #21956 (comment)

[Francesco-Lanciana]

Hey, I'm a little confused by the current with-apollo example. Midway through last year before the example was updated to use SSG it looked significantly different (Old method). It seems that the main difference between the old method and your current method is that the current requires you to manually import any queries that are used in the page and run them inside getServerSideProps, while the old method did a double render - first to run any queries and populate the cache (using getDataFromTree) and then to actually render the components. The old method was really nice in how you there were no extra steps, you write queries wherever they are needed and it just works. This new method seems like a lot more work for no real benefit.

What am I missing here? Is this new method better? And if so why?

Thanks in advance for the help!

[leerob]

@Francesco-Lanciana Overall, the older solution was brittle and error-prone. It was also very confusing for beginners. The new approach is much better IMO.

[filipesmedeiros]

@Francesco-Lanciana The old method relied on getDataFromTree, like you said, which IMO hides away the logic of fetching stuff and makes it confusing on what's being done CS and SS. I think like @leerob said, the example should focus on a more simple implementation: fetch stuff explicitly and pass it down to the client as normal Next page props.

But your approach of using gDFT also works fine and can probably be a better choice in some cases, just not for beginners (and in general) IMO

[Francesco-Lanciana]

@leerob what was brittle and error-prone about it? I definitely agree it was very confusing for beginners but this could have been fixed with better documentation. And the gDFT approach means you have to write a lot less boilerplate code once it was up and running. Not only that but this current example still relies on priming apollos cache during getServerSideProps to work, you don't actually make use of the props passed to the page directly. That's still going to confuse beginners.

@filipesmedeiros why do you think that this approach is better in general? You still had to pass ssr: true to the withApollo function in the previous approach to opt into SSR, and you didn't have to query things twice (once in getServerSideProps and once in the component).

It just feels like all this new approach does is make it slightly more explicit as to what data is being fetched server side at the expense of more boilerplate code. This might be useful if you want to selectively run only some queries when SSRing but I don't know why you would want to do that. And once you understand whats happening and your app becomes larger using gDFT seems like a natural next step to avoid this boilerplate. 100% the withApollo function with the gDFT approach was more involved but it's not like you should be having to modify that file often, and again with some extra documentation I don't think this extra render + gDFT method is hard to explain.

[filipesmedeiros]

Not only that but this current example still relies on priming apollos cache during getServerSideProps to work

It does, and I think that it shouldn't

That's still going to confuse beginners

Agreed

this new approach does is make it slightly more explicit as to what data is being fetched server side at the expense of more boilerplate code.

Disagree with "slightly" (everything is inside sSSP, instead of being scattered throughout several components. Arguably if it gets big enough you should isolate the data fetching in separate functions/files, but I still think it's way better), but it's not the only benefit. Like you said, being able to isolate which fetches should be done CS or SS, but also having a different data fetch "architecture", imagine this scenario: you have 3 queries, Q1, Q2 and Q3. The data returned from Q3 should be enough to populate/hydrate Q1 and Q2, but you don't want any component to use Q3 directly because no one component needs all that data. You can leave Q1 and Q2 in the component tree, but use Q3 in gSSP and use that data to hydrate Q1 and Q2 on the client. This can also apply to stuff like fetching with completely different methods like REST or whatever (although I believe this is irrelevant for the example, I think it's a more versatile pattern, and also less confusing)

Another benefit is no extra render, like you said (it can be bad, but don't think it generally will be.

Also, I just don't feel it feels Nexty ahah

[mansoor292]

Wow, this example just wasted hours because you guys forgot to actually pass the props down. Here I am thinking there is some magical implementation here that can replace implicitly populate useQuery from cache in real time.

FWIW I do not think this example is correctly titled. Should simply call this 'with-apollo-cache-rehydration' and leave the other one as 'with-apollo-GDT'

[filipesmedeiros]

@mansoor292 What do you mean? This is an issue, there's no implementation here

[mansoor292]

The way the with-Apollo example is written, the ssr page is not doing any ssr at all. The data itself is being cached and sent to client, and being rendered by/in the browser. Since it is also using a new Apollo client in getSSP, you don't even get to use the cache created server side.

On first read it seems like there is a 'magical' implementation. After 2 hours you find out the magic was there is no implementation at all.

This is essentially a security demo. How to not expose graphql api endpoint to a client. Kinda opinionated and not well documented.

My use case for next is to serve fast landing pages but then hydrate a full client side app.

[hinsxd]

@mansoor292 Having data pre-fetched on page load and directly showing data without loading state IS indeed SSR. SSR does not only mean "rendering html", but also mean prefetched data

[mansoor292]

While i think it’s definitely used as such, rendering cannot mean data fetching. To render means ‘cause to become’. Are we causing JavaScript to become a webpage by just preloading data? I personally don’t agree. In any case it’s a semantic discussion, better documentation, or better still just setting up the props to render the page would be better.

…

On Tue, May 11, 2021 at 12:01 AM Alvin Li ***@***.***> wrote: @mansoor292 <https://github.com/mansoor292> Having data pre-fetched on page load and directly showing data without loading state IS indeed SSR. SSR does not only mean "rendering html", but also mean prefetched data — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21984 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADPRYXWGBY2MQRJDYYIS5LTNCTZJANCNFSM4XKHFBOQ> .

[simontaisne]

@mansoor292 Not sure to understand 🤔 The ssr page in the current with-apollo example does render the prefetched data (queried in getServerSideProps) on the server-side and rehydrates the client-side cache.

[mbrowne]

I think some more documentation could definitely help here. I was surprised to find (with the help of console.log) that the creation of a new Apollo Client and rehydrating its cache happens not only in the browser, but also on the server. Is there a reason it's done this way instead of a more efficient solution?

To explain this a little better...in order for the server to return the initial HTML for the pages/ssr.js page in the example, the following sequence happens (all on the server, not involving the browser at all yet):

1. Execute getServerSideProps()
2. Create a new Apollo client
3. Execute the GraphQL query
4. Extract the contents of the Apollo Client cache and return them so they'll be available in the pageProps of _app.js
5. (Now in _app.js) call useApollo()
6. This causes a brand new Apollo Client instance to be created
7. Hydrate the cache of the new instance (using _apolloClient.cache.restore() ) with the data extracted earlier

Is there no way that the server couldn't simply reuse the existing instance instead of creating a new one and having to restore the cache? All the above steps happen as part of the same HTTP request.

[filipesmedeiros]

@mbrowne The client is in memory, you have to instantiate it in the browser in some way ahah You can't share instances between server and client (as far as I know ofc)

[mbrowne]

@filipesmedeiros I'm not talking about the browser at all. Both instances of ApolloClient I'm talking about are created on the server (and only exist on the server).

[filipesmedeiros]

Ahh didn't get that. Ok that's my bad. And I guess shouldn't happen yeah ApolloProvider allows you to pass in a client, maybe there's a way to recycle the same one? Maybe have a singleton created outside the scope of the page? Create a new one if it doesn't exist, each time you need it?

[mbrowne]

The tricky part would be to detect when to return a new ApolloClient and when not to. You definitely don't want to end up reusing the same ApolloClient on the server across multiple requests—that can cause cached data to leak from one page to another and cause hard-to-trace bugs. That's why there's this comment in the example code:

next.js/examples/with-apollo/lib/apolloClient.js

Line 53 in 3410106

// For SSG and SSR always create a new Apollo Client

It would be helpful to have some understanding of next.js internals (certainly more than I have) to know how to improve the example, if possible. From what I can gather, getServerSideProps() is deliberately kept quite separate from the rendering phase for some reason, even when the rendering is happening on the server—for one thing, the props that become pageProps have to be serialized to JSON just to be unserialized again prior to rendering _app.js.

[hinsxd]

Correct me if I am wrong, I think the tricky part is to make gSSP able to distinguish between SSR mode and client-side routing mode that returns a json. gSSP is required to be a separate named export for some reason, as it should be able to be triggered it any situation, and that's next internals decide how to deal with function result. Unless next internals provide some global context during SSR and/or pass it into gSSP, it is not quite possible to share some object instances.

[filipesmedeiros]

You can probably get access to some sort of request ID? And hopefully it's also accessible on the Page component? Just guessing though

[sebas5384]

I have being using the old example for some long time (maybe a couple of years) on production of massive sites and using getDataFromTree and were never the case of some sort of performance issue related to the double-render need for SSR.

This change forces developers to imperatively fetch and know how to fetch any content needed to fullfil the page's data to render, also the execution of queries out of the components (or containers) will create a new layer of coupling outside of it creating a lot of boilerplate. Maybe for a small site with a couple of pages and simple components which don't need to render on server the new example seems ok, though I don't see how it could scale when adding more complex components.

On the case of SSG the next-with-apollo module solves it by removing the getDataFromTree as default and adding it as an option.
Also there's an issue on this module about supporting getDataFromTree also in SSG mode.

I think a new example should be created with two different goals, one with the new simple and flexible way and another with better SSR DX (in my opinion) using next-with-apollo module or alike.

[github-actions]

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.