-
Notifications
You must be signed in to change notification settings - Fork 27.6k
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
Update withApollo example #10451
Update withApollo example #10451
Conversation
We can get rid of .rewind by now as the latest next/head no longer uses legacy context.
60a75d5
to
186479f
Compare
Stats from current PRDefault Server ModeGeneral
Client Bundles (main, webpack, commons)
Client Bundles (main, webpack, commons) Modern
Legacy Client Bundles (polyfills)
Client Pages
Client Pages Modern
Client Build Manifests
Rendered Page Sizes
Serverless ModeGeneral
Client Bundles (main, webpack, commons)
Client Bundles (main, webpack, commons) Modern
Legacy Client Bundles (polyfills)
Client Pages
Client Pages Modern
Client Build Manifests
Serverless bundles
Commit: 186479f |
Stats from current PRDefault Server ModeGeneral
Client Bundles (main, webpack, commons)
Client Bundles (main, webpack, commons) Modern
Legacy Client Bundles (polyfills)
Client Pages
Client Pages Modern
Client Build Manifests
Rendered Page Sizes
Serverless ModeGeneral
Client Bundles (main, webpack, commons)
Client Bundles (main, webpack, commons) Modern
Legacy Client Bundles (polyfills)
Client Pages
Client Pages Modern
Client Build Manifests
Serverless bundles
Commit: 565b739 |
// to the component, otherwise the component would have to call initApollo() again but this | ||
// time without the context, once that happens the following code will make sure we send | ||
// the prop as `null` to the browser | ||
apolloClient.toJSON = () => null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I did this in next-with-apollo a long time ago, I don't remember why I never put it here 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, is this required? I did it as a bug fix for a configuration in the package, but I don't know about this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason is, it feels very hacky, the overhead of creating a new instance of the Apollo Client may not matter (otherwise go ahead)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy for this to be fixed in a follow-up! It's likely fixing an already existing bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lfades This line is there to prevent the following issue.
I think it is a genius hack that reduces a lot of complexity inside this HOC.
I have to admit that this was not my idea. I believe I saw this pattern inside your excellent lfades/next-with-apollo module, the first time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @HaNdTriX!
Stats from current PRDefault Server ModeGeneral
Client Bundles (main, webpack, commons)
Client Bundles (main, webpack, commons) Modern
Legacy Client Bundles (polyfills)
Client Pages
Client Pages Modern
Client Build Manifests
Rendered Page Sizes
Serverless Mode (Increase detected
|
zeit/next.js canary | HaNdTriX/next.js examples/apollo-easy-config | Change | |
---|---|---|---|
buildDuration | 12.8s | 12.4s | -384ms |
nodeModulesSize | 52.9 MB | 52.9 MB | ✓ |
Client Bundles (main, webpack, commons)
zeit/next.js canary | HaNdTriX/next.js examples/apollo-easy-config | Change | |
---|---|---|---|
main-HASH.js gzip | 5.13 kB | 5.13 kB | ✓ |
webpack-HASH.js gzip | 746 B | 746 B | ✓ |
4952ddcd88e7..54d3.js gzip | 4.68 kB | 4.68 kB | ✓ |
commons.HASH.js gzip | 4.06 kB | 4.06 kB | ✓ |
de003c3a9d30..c95c.js gzip | 13.6 kB | 13.6 kB | ✓ |
framework.HASH.js gzip | 39.1 kB | 39.1 kB | ✓ |
Overall change | 67.4 kB | 67.4 kB | ✓ |
Client Bundles (main, webpack, commons) Modern
zeit/next.js canary | HaNdTriX/next.js examples/apollo-easy-config | Change | |
---|---|---|---|
main-HASH.module.js gzip | 4.13 kB | 4.13 kB | ✓ |
webpack-HASH..dule.js gzip | 746 B | 746 B | ✓ |
4952ddcd88e7..dule.js gzip | 5.56 kB | 5.56 kB | ✓ |
de003c3a9d30..dule.js gzip | 12.4 kB | 12.4 kB | ✓ |
framework.HA..dule.js gzip | 39.1 kB | 39.1 kB | ✓ |
Overall change | 62 kB | 62 kB | ✓ |
Legacy Client Bundles (polyfills)
zeit/next.js canary | HaNdTriX/next.js examples/apollo-easy-config | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 4.76 kB | 4.76 kB | ✓ |
Overall change | 4.76 kB | 4.76 kB | ✓ |
Client Pages
zeit/next.js canary | HaNdTriX/next.js examples/apollo-easy-config | Change | |
---|---|---|---|
_app.js gzip | 1.15 kB | 1.15 kB | ✓ |
_error.js gzip | 4.07 kB | 4.07 kB | ✓ |
hooks.js gzip | 779 B | 779 B | ✓ |
index.js gzip | 222 B | 222 B | ✓ |
link.js gzip | 2.89 kB | 2.89 kB | ✓ |
routerDirect.js gzip | 283 B | 283 B | ✓ |
withRouter.js gzip | 282 B | 282 B | ✓ |
Overall change | 9.68 kB | 9.68 kB | ✓ |
Client Pages Modern
zeit/next.js canary | HaNdTriX/next.js examples/apollo-easy-config | Change | |
---|---|---|---|
_app.module.js gzip | 576 B | 576 B | ✓ |
_error.module.js gzip | 3.06 kB | 3.06 kB | ✓ |
hooks.module.js gzip | 371 B | 371 B | ✓ |
index.module.js gzip | 212 B | 212 B | ✓ |
link.module.js gzip | 2.46 kB | 2.46 kB | ✓ |
routerDirect..dule.js gzip | 273 B | 273 B | ✓ |
withRouter.m..dule.js gzip | 272 B | 272 B | ✓ |
Overall change | 7.22 kB | 7.22 kB | ✓ |
Client Build Manifests
zeit/next.js canary | HaNdTriX/next.js examples/apollo-easy-config | Change | |
---|---|---|---|
_buildManifest.js gzip | 61 B | 61 B | ✓ |
_buildManife..dule.js gzip | 61 B | 61 B | ✓ |
Overall change | 122 B | 122 B | ✓ |
Serverless bundles Overall increase ⚠️
zeit/next.js canary | HaNdTriX/next.js examples/apollo-easy-config | Change | |
---|---|---|---|
_error.js gzip | 290 kB | 290 kB | -115 B |
404.html gzip | 1.44 kB | 1.44 kB | ✓ |
hooks.html gzip | 1.08 kB | 1.08 kB | ✓ |
index.js gzip | 289 kB | 289 kB | -115 B |
link.js gzip | 320 kB | 319 kB | -634 B |
routerDirect.js gzip | 316 kB | 316 kB | |
withRouter.js gzip | 315 kB | 317 kB | |
Overall change | 1.53 MB | 1.53 MB |
Commit: ed94c45
Stats from current PRDefault Server ModeGeneral
Client Bundles (main, webpack, commons)
Client Bundles (main, webpack, commons) Modern
Legacy Client Bundles (polyfills)
Client Pages
Client Pages Modern
Client Build Manifests
Rendered Page Sizes
Serverless Mode (Decrease detected ✓)General
Client Bundles (main, webpack, commons)
Client Bundles (main, webpack, commons) Modern
Legacy Client Bundles (polyfills)
Client Pages
Client Pages Modern
Client Build Manifests
Serverless bundles Overall decrease ✓
Commit: 16d456e |
Stats from current PRDefault Server ModeGeneral
Client Bundles (main, webpack, commons)
Client Bundles (main, webpack, commons) Modern
Legacy Client Bundles (polyfills)
Client Pages
Client Pages Modern
Client Build Manifests
Rendered Page Sizes
Serverless Mode (Decrease detected ✓)General
Client Bundles (main, webpack, commons)
Client Bundles (main, webpack, commons) Modern
Legacy Client Bundles (polyfills)
Client Pages
Client Pages Modern
Client Build Manifests
Serverless bundles Overall decrease ✓
Commit: baca121 |
Decent update, many thanks @HaNdTriX 👏 Looking forward to switching over to your implementation in a few days. Will let know if anything breaks or gets confusing 🙌 |
Love it! Switched it with the recommended Auth version above. Everything works and more consise. One thing to note: are there any plans to make the Auth version available outside of this PR for documentation purposes? Seems like a 'with-apollo-auth' example could be useful, or simply add it in comments in the main folder. Just my 2 cents. |
@BiscuiTech Currently I am not a fan of adding more examples regarding apollo. Maybe we can add a docs directory where we explain different strategies regarding those PRs.
As soon as I have time I'll try to write down some of the thoughts I currently have in my head. |
Id be particularly interested in your thoughts on this. I understood JS cookies to be less secure so have taken an approach using a seperate login/logout API and have Apollo server pull the token from the cookie. |
@HaNdTriX that would be awesome, it would be incredibly helpful!! Regarding the current PR I've personally encountered an error but I haven't found a way to reproduce it yet. I'm guessing it's because I'm handling auth with Auth0 but I'm uncertain of this. Perhaps it's more likely a bug with the implementation I'm using will need to dig deeper. The error occurs locally when I'm logging into the web app, then it crashes with this:
But it's weird because the Auth0 config is done as in the example repo instructions and it was working perfectly before updating the apollo config (it was the only change). So I'm puzzled at the moment. Will let you know if I find something. |
Totally my bad. Ignore the previous one, complete false alarm. Everything is working nicely. Thanks again! |
Hi - thanks for the update, I just tested it to see if the new version maybe solve the problem, that
Even if the error is visible through CSR the hole part of SSR is somehow useless without an option to handle errors. Most likely it is a problem with apollo and the issue should be fixed with apollo client 3.0 which is currently in beta. Cause you properly will update the example I mention this here - there are many issues I found somehow related to that problem, like this one: apollographql/react-apollo#3678 |
@Linux249 thanks for your feedback. I will try to look into the issue regarding errors in the future. A good reproduction example could help a lot. I've also already looked into
So lets wait for a stable release until we update this example. |
|
||
// getDataFromTree does not call componentWillUnmount | ||
// head side effect therefore need to be cleared manually | ||
Head.rewind() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to @timneutkens , this should not have been removed: #9326 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying 🙏. #10696
Here is another update on the withApollo example. Using apollo with Next.js not trivial. Finding the right abstractions is the key for the community to adopt such a technologie.
Changelog
Head.rewind()
from HOC.(next/head
no longer uses legacy context).apolloClient
when rendering on the server.ssr: false
the default. Users of this HOC now need to explicitly enable datafetching using slowAppTree
traversal.ApolloClient
configuration into a separate file (./apolloClient.js
). This simplifies the setup dramatically, since the developer doesn't need to care about the Next.js peculiarities. Implementing Server Side Auth is now much easier, since we are now able to access theNextPageContext
right fromcreateApolloClient
.initOnContext
method. Just incase a user want's to use theapolloClient
insidegetStaticProps
orgetServerProps
. We can now manually install it on theNextPageContext
.apolloState
if created ingetStaticProps
Examples
Setting up ApolloClient
Adding authentication
Using
withApollo
globally on_app.js
File:
pages/_app.js
Using ApolloClient inside
getStaticProps