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.