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.

Sign up for GitHub

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

Jump to bottom

Fix getInitialProps issue in with-apollo example #8620

Merged
merged 8 commits into from
Sep 4, 2019
Merged

Fix getInitialProps issue in with-apollo example #8620

merged 8 commits into from
Sep 4, 2019

Conversation

HaNdTriX
Copy link
Contributor

@HaNdTriX HaNdTriX commented Sep 4, 2019 •
edited
Loading

Motivation

The code before had three design flaws:

  • When we skip WithApollo.getInitialProps we must hoist PageComponent.getInitialProps if it is present. (Special thanks to @justinnipper for pointing me into the right direction 🙏.)
  • We should expose the apolloClient to the underlying PageComponent.getInitialProps contexts. This way users are able to use apollo inside PageComponent.getInitialProps to perform prerender redirects or similar. (e.g. await ctx.apolloClient.query(...))
  • We used the apollo-boost package wich was optimized for client-usage.

Changelog

  • Fix getInitialProps hoisting
  • Expose apolloClient in pageContext
  • Disable connectToDevTools in production
  • Remove apollo-boost package
  • Remove author entry from package.json
  • Simplify fetch polyfill
  • Add redirect abort check
  • Added no-ssr example
  • Added some docs

Status: ready
Related: #8508, #8504

@HaNdTriX HaNdTriX mentioned this pull request Sep 4, 2019
Merged
@HaNdTriX HaNdTriX force-pushed the examples/with-apollo branch 2 times, most recently from 366183b to 6c2f5c4 Compare September 4, 2019 09:09
@huv1k
Copy link
Contributor

huv1k commented Sep 4, 2019

Hey @HaNdTriX, thanks for another nice catch and addition :) I was thinking more about this example and I came to conclusion we could display more pages, with different settings so this example shows all possibilities. What do you think?

HaNdTriX added a commit to HaNdTriX/next.js that referenced this pull request Sep 4, 2019
@HaNdTriX
Add client only example page
b989366 
vercel#8620 (comment)
HaNdTriX added a commit to HaNdTriX/next.js that referenced this pull request Sep 4, 2019
@HaNdTriX
Add client only example page
fbfd6de 
vercel#8620 (comment)
@HaNdTriX
Copy link
Contributor Author

HaNdTriX commented Sep 4, 2019 •
edited
Loading

@huv1k I like the idea. I've added the following:

1. IndexPage

Code

...
export default withApollo(IndexPage)

PageScreenshot

localhost_3000_

2. ClientPage

Code

...
export default withApollo(ClientOnlyPage, {
  // Disable apollo ssr fetching in favour of automatic static optimization
  ssr: false
})

PageScreenshot

localhost_3000_client-only

(Please note that this screenshot will change after apollo fetched on the client.)

Result

Page              Size     Files  Packages
┌ σ /             159 kB       8        23
├   /_app         1.83 kB      0         3
├   /_document
├   /_error       2.03 kB      0         3
├ ⚡ /about        1.8 kB       2         3
└ ⚡ /client-only  159 kB       8        23

I am not sure if I want to add an getInitialProps example since I don't want people to overuse this technique.

HaNdTriX added a commit to HaNdTriX/next.js that referenced this pull request Sep 4, 2019
@HaNdTriX
Add client only example page
6b7b31e 
vercel#8620 (comment)
@HaNdTriX
Fix getInitialProps in with-apollo example
4a0c4b9 
The code before had two design flaws:

* When we skip WithApollo.getInitialProps we must hoist PageComponent.getInitialProps if it is present.
* We should expose the apolloClient to underlying PageComponent.getInitialProps contexts.
@HaNdTriX
Add abort check
9545cca
@HaNdTriX
Add some comments
5ccc359
@HaNdTriX
Add client only example page
64b74fd 
vercel#8620 (comment)
@HaNdTriX
Remove connectToDevTools setting in favor default config
8af41b1 
https://github.com/apollographql/apollo-client/blob/7eaf4132cd2cd6244260777799406aaa03fcf377/packages/apollo-client/src/ApolloClient.ts#L170-L173
@HaNdTriX
Remove fetch check
40f1af1 
This is done by https://www.npmjs.com/package/isomorphic-unfetch
@HaNdTriX
Remove apollo-boost
549a03e 
I am removing this package, because we never actually used it.
This is because we use the named export of apollo boost wich resolves to apollo-client.

This way we removed apollo-link-state, apollo-link-error
@huv1k
Copy link
Contributor

huv1k commented Sep 4, 2019

@HaNdTriX great job as always 🚀

@timneutkens timneutkens changed the title Fix getInitialProps design flaws in with-apollo example Fix getInitialProps issue in with-apollo example Sep 4, 2019
NipperInShorts
NipperInShorts approved these changes Sep 4, 2019
View reviewed changes
Copy link

@NipperInShorts NipperInShorts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. thank you!

ijjk
ijjk approved these changes Sep 4, 2019
View reviewed changes
Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@ijjk ijjk merged commit cba5c05 into vercel:canary Sep 4, 2019
@huv1k huv1k added this to the 9.0.6 milestone Sep 4, 2019
@HaNdTriX HaNdTriX deleted the examples/with-apollo branch September 4, 2019 22:57
This was referenced Sep 5, 2019
Merged
Merged
@adamsoffer adamsoffer mentioned this pull request Sep 25, 2019
Closed
guibernardino added a commit to guibernardino/next-apollo that referenced this pull request Oct 31, 2019
@guibernardino
Expose apolloClient in pageContext
a1403d6 
For more information check vercel/next.js#8620
@guibernardino guibernardino mentioned this pull request Oct 31, 2019
Merged
adamsoffer pushed a commit to adamsoffer/next-apollo that referenced this pull request Nov 7, 2019
@guibernardino @adamsoffer
Expose apolloClient in pageContext
632a9df 
For more information check vercel/next.js#8620
@vercel vercel locked as resolved and limited conversation to collaborators Feb 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ijjk ijjk ijjk approved these changes

@huv1k huv1k huv1k approved these changes

@NipperInShorts NipperInShorts NipperInShorts approved these changes

@lfades lfades Awaiting requested review from lfades

@Timer Timer Awaiting requested review from Timer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
9.0.6
Development

Successfully merging this pull request may close these issues.
4 participants
@HaNdTriX @huv1k @NipperInShorts @ijjk