Async onClientEntry #1735
Async onClientEntry #1735
Conversation
|
Deploy preview ready! Built with commit 42e3bb3 |
|
Deploy preview ready! Built with commit 42e3bb3 |
There was a problem hiding this comment.
Looking good!
Gatsby runs its APIs in "series". This PR breaks that contract currently as it runs APIs in parallel. Here's a good looking pattern for running Promises serially https://stackoverflow.com/a/42324940
Working today on landing #1503 which will probably cause some breaking changes you'll need to update this for.
Since this PR makes Gatsby require Promise support, we'll need to document this. To this point, Gatsby had tracked React's browser support i.e. IE9+. We need to decide also if we should provide a Promise polyfill by default. I'd lean towards yes given IE11 is still ~5% of (US) traffic and 3% of global.
packages/gatsby/src/cache-dir/app.js
Outdated
| navigator.serviceWorker.getRegistrations().then(registrations => { | ||
| for (let registration of registrations) { | ||
| registration.unregister() | ||
| // Troubles with this import in the past? |
There was a problem hiding this comment.
This is because es6/commonjs modules work differently.
packages/gatsby/src/cache-dir/app.js
Outdated
| let NextRoot = require(`./root`) | ||
|
|
||
| if (NextRoot.default) { | ||
|
|
There was a problem hiding this comment.
is the Github diff display being weird or is there really spaces between all these lines? If so, those need removed.
There was a problem hiding this comment.
Apologies, some of my local formatting sneaking in, I'll strip them out.
|
Definitely. I've done this pattern in ptotoculture as all service providers similarly boot in series. Want me to wait until the other change lands? This has been fairly trivial to add. |
|
Go ahead with the docs plus promise polyfill. Not sure yet exactly how the other PR will affect this one. |
|
|
Updated, ready for another look. |
docs/docs/browser-support.md
Outdated
| @@ -1,7 +1,22 @@ | |||
| --- | |||
There was a problem hiding this comment.
You'll also need to update the doc-links.yaml file with the new link.
docs/docs/browser-support.md
Outdated
| @@ -1,7 +1,22 @@ | |||
| --- | |||
| title: Browserslist | |||
| title: Browser Support | |||
| --- | |||
|
|
|||
There was a problem hiding this comment.
Add/replace the text at the start saying "Gatsby supports the same browsers as the stable version of React.js which is currently IE9+ as well as the most recent versions of other popular browsers. Gatsby requires support for the Promise API which is not supported in older browsers. Because of this, Gatsby…"
| @@ -40,6 +40,11 @@ module.exports = (state = {}, action) => { | |||
| action = _.set(action, [`payload`, `pathPrefix`], ``) | |||
| } | |||
|
|
|||
| // Default polyfill to true. | |||
| if (!_.has(action, `polyfill`)) { | |||
There was a problem hiding this comment.
This should be on the payload.
|
Looked at gatsbyjs.org's preview build for this PR and saw a bunch of errors https://deploy-preview-1735--gatsbyjs.netlify.com/
|
|
I wonder if that's related to what I was seeing on a site...I was having no luck getting webpacks DefinePlugin to insert anything. May be completely unrelated! |
|
Yeah, I've been trying to figure out if these tests are giving false negatives or not as things seem to still be working locally. If I have to do anything for them, let me know. |
|
Looks like the gatsbyjs.org isn't erroring anymore but the layout isn't being added |
|
@KyleAMathews - Hey, sorry. Not to nag, but is there anything you're waiting for on this one? GitHub still seems to be showing "changes requested", although I have updated my branch with the changes you asked for last. |
|
Thanks for your work here! |
I have to manually change the status of my review and since I hadn't had time to review things the status was still the same. Please understand that your PRs and issues are not always going to reviewed quickly. It doesn't mean you're doing anything wrong it just means I have other stuff going on that doesn't always allow me to spend the hours it takes to review and land PRs. So comments like this and asking on Discord are very much nagging and I would appreciate you just assume that your PR will get reviewed and landed ASAP and wait patiently until then. We'd love to have more maintainers to speed up landing PRs and I'm perfectly willing to mentor you and others who want to level up to being able to maintain parts of Gatsby but in the meantime, sometimes things will be a bit choppy. |
|
No problems, I definitely wasn't intending it. More just trying to be prompt in accommodating your suggestions - promise! Thank you so much! |
|
I do appreciate that :-) |
|
But to be clear. I don't expect y'all to be prompt necessarily either. Often this isn't your job or whatever so please don't feel guilty if you don't have time to finish something up. If it's that important then someone else will pick it up sooner or later. |
|
Hiya @atrauzzi! 👋 This is definitely late, but on behalf of the entire Gatsby community, I wanted to say thank you for being here. Gatsby is built by awesome people like you. Let us say “thanks” in two ways:
If you have questions, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’. Thanks again! 💪💜 |
Tested locally and I'm getting async support in my bootstrap flows during development now.
Let me know what steps I need to take to ensure I'm abiding by gatsby's standards. I think I also will have to update the static app bootstrap as well?