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.

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

feat(gatsby): enable modern builds for gatsby #14289

Closed
wants to merge 16 commits into from

Conversation

wardpeet
Copy link
Contributor

@wardpeet wardpeet commented May 24, 2019

Description

Gatsby generates 2 bundles. One .js & one .mjs, the .js will be used for legacy browsers (<script nomodule>) & mjs will be used for es supported modules.

Changes I made:

  • remove process.env from babel-preset-env, use options instead.
  • add modern option to babel-preset-env
  • enable js & mjs bundling by webpack
  • add module, nomodule script tags to pages (make sure we fix the ios/safari bug)
  • add option do disable modern builds
  • add e2e tests

I wonder if we should detect if a given browserlist is already > modern zo we don't have to add 2 script tags. Perhaps just disable when a custom babelrc and browserlist are given?

@DSchau created a killer todo, so for more info:
#2114 (comment)

Related Issues

#2114

Note

We lose preload for legacy browsers on first visit when a modern build is generated because there is no way for us to conditionally preload one set (legacy or modern) before knowing the browser and by then it is perhaps too late to add preload tags

@KyleAMathews
Copy link
Contributor

Great work! Is the idea we'll release this behind a command-line flag / experimental config value?

wardpeet added 2 commits May 25, 2019 00:09
- add module scripts to static build html
- make sure prefetch works with modern files
- combine modern & legacy stats together
@wardpeet wardpeet force-pushed the feat/modern-builds branch from c8c5b4c to 39a60a8 Compare May 24, 2019 22:09
@wardpeet
Copy link
Contributor Author

I was thinking of writing an issue about creating a new options inside gatsby-config.js called _experiments where we could have these kinds of feature flags (Release Toggles).

@DSchau
Copy link
Contributor

DSchau commented May 24, 2019

@wardpeet please do! I've been thinking that would be really valuable--basically a way that we can get more fine-grained control (and likely analytics data, as well) of these experiments and make them a little more user-friendly than setting an environment variable.

(It shouldn't necessarily block this though--but I do love the idea!)

Copy link
Contributor Author

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Added some comments for myself so I don't forget XD

@@ -275,6 +298,8 @@ export default (pagePath, callback) => {
scripts
.slice(0)
.reverse()
// remove legacy scripts from our preload as we preload our modern files instead
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure if there is a workaround for this that we can have our cake and eat it too. A way to do this is document.write but I rather don't want to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's add a disclaimer to the PR to say that we lose preload for legacy browers on first run

Copy link
Contributor

Choose a reason for hiding this comment

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

Added the note!

})

// Patches browsers who have a flawed noModule - module system
// Sadly we lose preload for legacy browsers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
// Sadly we lose preload for legacy browsers
// Sadly we lose the benefit of the preload scanner for legacy browsers
// because the the sources are hidden in javascript.

// @see https://caniuse.com/#feat=es6-module
// 1. Safari 10.1 supports modules, but does not support the `nomodule` attribute - it will load <script nomodule> anyway.
// 2. Edge does not executes noModule but still fetches it
const noModuleBugFixScripts = `(function(b){function c(e){var d=b.createElement("script");d.src=e;b.body.appendChild(d)}"noModule"in b.createElement("script")||/Version\\/10\\.1(\\.\\d+)* Safari|Version\\/10\\.\\d(\\.\\d+)*.*Safari|Edge\\/1[6-8]\\.\\d/i.test(navigator.userAgent)||(%scripts%)})(document)`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had no clue on how to write this in a nicer way. Basically it detects if noModule exists or it does not exists and is safari 10.1.x or safari ios 10.x as they do have know type="module" but not nomodule

Suggested change
const noModuleBugFixScripts = `(function(b){function c(e){var d=b.createElement("script");d.src=e;b.body.appendChild(d)}"noModule"in b.createElement("script")||/Version\\/10\\.1(\\.\\d+)* Safari|Version\\/10\\.\\d(\\.\\d+)*.*Safari|Edge\\/1[6-8]\\.\\d/i.test(navigator.userAgent)||(%scripts%)})(document)`
const noModuleBugFixScripts = `(function(b){function c(e){var d=b.createElement("script");d.src=e;b.body.appendChild(d)}"noModule"in b.createElement("script")||/Version\\/10\\.1(\\.\\d+)* Safari|Version\\/10\\.\\d(\\.\\d+)*.*Safari/i.test(navigator.userAgent)||(%scripts%)})(document)`

@wardpeet
Copy link
Contributor Author

@DSchau I think it's best to keep this under an experiment for now, I have no idea if this is going to be flawless 😄 for all websites.

@sidharthachatterjee
Copy link
Contributor

sidharthachatterjee commented Jun 3, 2019

I think you should document 3 things on the blog post/documentation page:

  • that we "handle" the Safari bug with the dynamic script tag addition
  • the Chrome Core JS catastrophe
  • Side note for folks to beware if they have custom plugin code that reads webpack.stats.json while mentioning that we handle the official ones (guess-js, postcss, netlify cms, netlify headers, offline plugin)

Once per page manifest lands, we have to update this (cc @Moocar )

Random thought: In the future, a build that is set using browserslist might be "more modern" than modern so we might need to be careful about naming here 😉

@moonmeister
Copy link
Contributor

This may be helpful:

https://jasonformat.com/modern-script-loading/

@wardpeet
Copy link
Contributor Author

Thanks @moonmeister! I will indeed thinker a bit about that blog post

createComponentUrls(resourceName).map(url => prefetchHelper(url))
)
let componentUrls = createComponentUrls(resourceName)
if (process.env.MODERN_BUILD) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lets filter inside static-entry to only save modern / legacy assets inside the window.___chunkMapping variable

@@ -275,6 +298,8 @@ export default (pagePath, callback) => {
scripts
.slice(0)
.reverse()
// remove legacy scripts from our preload as we preload our modern files instead
Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's add a disclaimer to the PR to say that we lose preload for legacy browers on first run

@sidharthachatterjee sidharthachatterjee marked this pull request as ready for review July 17, 2019 11:20
@sidharthachatterjee sidharthachatterjee requested review from a team as code owners July 17, 2019 11:20
@sidharthachatterjee sidharthachatterjee self-assigned this Jul 17, 2019
@sidharthachatterjee sidharthachatterjee self-requested a review July 17, 2019 11:21
@sidharthachatterjee sidharthachatterjee added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Jul 17, 2019
@wardpeet wardpeet removed the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Jul 17, 2019
Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

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

Looking great overall! Thank you @wardpeet ❤️

Left a few small comments

@@ -0,0 +1,3 @@
# production-runtime
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# production-runtime
# modern-builds

@@ -0,0 +1,3 @@
# production-runtime

A Gatsby project to test our production runtime for Themes
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
A Gatsby project to test our production runtime for Themes
A Gatsby project to test out modern builds

@@ -0,0 +1,5 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

File can be deleted since it isn't used

@@ -0,0 +1,17 @@
// ***********************************************************
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably get rid of this folder and its contents as well

@@ -0,0 +1,34 @@
{
"name": "production-runtime",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"name": "production-runtime",
"name": "modern-builds",

"name": "production-runtime",
"description": "Gatsby default starter",
"version": "1.0.0",
"author": "Sidhartha Chatterjee <sid@gatsbyjs.com>",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"author": "Sidhartha Chatterjee <sid@gatsbyjs.com>",
"author": "Ward Peeters <ward@gatsbyjs.com>",

e2e-tests/modern-builds/package.json Outdated Show resolved Hide resolved
e2e-tests/modern-builds/package.json Outdated Show resolved Hide resolved
packages/gatsby/cache-dir/static-entry.js Outdated Show resolved Hide resolved
packages/gatsby/cache-dir/static-entry.js Outdated Show resolved Hide resolved
@ChristopherBiscardi
Copy link
Contributor

Looks like this takes advantage of webpack multi-compiler yeah? Might affect gatsby-plugin-mdx's html field resolution. Would love a ping whenever there's a testable release.

@wardpeet
Copy link
Contributor Author

I'm going to restructure this PR in small chunks and probably remove the multi compiler instance as it's probably going to break things.

I'll close this PR for now and re-open smaller chunks. I'll keep the branch alive.

@wardpeet wardpeet closed this Aug 19, 2019
@wardpeet wardpeet deleted the feat/modern-builds branch September 23, 2020 18:50
@wardpeet wardpeet restored the feat/modern-builds branch September 23, 2020 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants