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 babel-loader for all dependencies #14111

Merged
merged 7 commits into from Jun 27, 2019

Conversation

wardpeet
Copy link
Contributor

@wardpeet wardpeet commented May 17, 2019

Description

I ran gatsby through babel-loader to make sure babel-preset-env can adds it's polyfills for all dependencies so we don't break on older browsers. This will also help us for modern builds so our modern build will have fewer polyfills than our classic one.

To give you an idea on how much slower it is. The first run on a big website takes about double so that's something to wonder about.

default-starter no babel cache babel cache
transpile node_modules (no babel-cache) 5,5623s 2,887s
exclude node_modules (no babel-cache) 3,148s 2,224s
diff 76% 29%
gatsbyjs.org no babel cache babel cache
transpile node_modules (no babel-cache) 31,487s 14,684s
exclude node_modules (no babel-cache) 15,896s 10,804s
diff 98% 36%
default-starter runtime 1 runtime 2 runtime 3
transpile node_modules (no babel-cache) 5,198s 5,768s 5,721s
transpile node_modules (babel-cache) 2,771s 2,958s 2,932s
exclude node_modules (no babel-cache) 2,937s 3,339s 3,168s
exclude node_modules (babel-cache) 2,172s 2,283s 2,217s
gatsbyjs.org runtime 1 runtime 2 runtime 3
transpile node_modules (no babel-cache) 30.013s 32.846s 31.602s
transpile node_modules (babel-cache) 14.761s 15.519s 13.771s
exclude node_modules (no babel-cache) 15.686s 16.385s 15.617s
exclude node_modules (babel-cache) 12.336s 10.532s 9.545s

bundle info

http://wardpeet-filestorage.surge.sh/gatsby-store/pr-14111

Related Issues

#12409
#12399
#13427
#5553
#7064
#13241

@@ -292,7 +292,7 @@ module.exports = async ({
let js = (options = {}) => {
return {
test: /\.jsx?$/,
exclude: vendorRegex,
exclude: /core-js/,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow core-js needs to be excluded because babel will try to polyfill core-js with core-js XD also mini-extra-css-plugin doesn't work well without this. This magically fixes it, i'm not 100% sure so the above comments are just speculation 😄 .

I'll add a comment when it's getting out of draft mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

will try to polyfill core-js with core-js

😂

@sidharthachatterjee
Copy link
Contributor

This will finally allow dependencies (themes and others) to have queries! Woohoo 🚀

@sidharthachatterjee
Copy link
Contributor

Want to also post diff of bundle size from before and after? Curious how much this adds or removes

@sidharthachatterjee
Copy link
Contributor

Can also remove note in https://www.gatsbyjs.org/docs/browser-support/#polyfills

@wardpeet
Copy link
Contributor Author

wardpeet commented May 17, 2019

I've added a list of changes that this PR creates. It only adds 12kb to gatsbyjs.org which is extremely low minified so I guess we should just get this in and reduce it again when using modern builds.

Babel all the things!

http://wardpeet-filestorage.surge.sh/gatsby-store/pr-14111/

@wardpeet wardpeet added this to In progress in OSS Roadmap via automation Jun 4, 2019
@wardpeet wardpeet added the status: awaiting author response Additional information has been requested from the author label Jun 4, 2019
@wardpeet
Copy link
Contributor Author

wardpeet commented Jun 7, 2019

I've edited above comment with new elements! Lets get this in! ^^

@wardpeet wardpeet added status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response and removed status: awaiting author response Additional information has been requested from the author labels Jun 7, 2019
@wardpeet wardpeet marked this pull request as ready for review June 7, 2019 22:18
@wardpeet wardpeet requested a review from a team as a code owner June 7, 2019 22:18
@KyleAMathews
Copy link
Contributor

💯yeah, let's do this! The extra build time is a bummer but so is sites not working on older browsers.

Does this PR disable transpiling node_modules during development? That'd be something to consider as people are more sensitive to build times when trying to start the develop server vs. doing a production build (which probably is happening on a CI server somewhere)

@wardpeet
Copy link
Contributor Author

I've found a bug on develop where somehow react-hot-loader is causing issues. trying to find out why

@wardpeet wardpeet requested review from a team June 11, 2019 14:01
@wardpeet wardpeet requested review from a team as code owners June 11, 2019 14:01
@wardpeet
Copy link
Contributor Author

seems like I needed to add event-source-polyfill & webpack-hot-middleware/client as we don't want them to get polyfilled. As they act as HMR polyfills. If we don't exclude we get some core-js loops and errors.

@wardpeet
Copy link
Contributor Author

I haven't tested the latest change, I'll do that tomorrow

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.

I think this is ready to go now! Thank you @wardpeet ❤️

@wardpeet wardpeet merged commit 268ed27 into gatsbyjs:master Jun 27, 2019
OSS Roadmap automation moved this from In progress to Done Jun 27, 2019
@wardpeet wardpeet deleted the feat/babel-transpile-all branch June 27, 2019 06:19
@wardpeet
Copy link
Contributor Author

Published in gatsby@2.11.0

@monsieurnebo
Copy link
Contributor

That's great, thanks for this @wardpeet !

ChristopherBiscardi added a commit to ChristopherBiscardi/gatsby that referenced this pull request Jun 27, 2019
Because of gatsbyjs#14111, we can remove the custom theme processing.
@jquense
Copy link
Contributor

jquense commented Jun 27, 2019

this is kinda of a breaking change for sites with custom babel configs ya'll

@DSchau
Copy link
Contributor

DSchau commented Jun 27, 2019

@jquense yikes —- we’re sorry to hear that!

Would you mind opening up an issue and showing us exactly how this is breaking?

@jquense
Copy link
Contributor

jquense commented Jun 27, 2019

It wasn't a huge deal for us, but it's not obvious what was happening. There isn't a lot to how it breaks unfortunately. It's just not safe to apply any babel config to any code.

For instance: babel-dev-expression breaks trying to compile warning

@joealden
Copy link

@wardpeet the increased build times are a pretty big issue for us, as our build times were long enough as they were. They have gone from about 6 minutes all the way up to 11 minutes (and rebuilds are triggered quite regularly). As we are paying for compute time per minute spent building, this drives up the costs for CI builds to nearly double.

While I understand why the changes in this PR were introduced, would it not be possible to have an option to turn this off (in gatsby-config.js or as a env var)?

@wardpeet
Copy link
Contributor Author

@joealden thanks, i've created an issue about this #15213

rdela added a commit to rdela/voidcluster that referenced this pull request Jun 28, 2019
Gatsby@2.11.0 Build failure
gatsbyjs/gatsby#15190 (comment)
> gatsby-plugin-netlify-cms runs an independent Webpack build within
> Gatsby's Webpack build (insert "yo dawg" reference here) using a
> modified version of the Gatsby Webpack config. Gatsby 2.11.0 no longer
> excludes node_modules, which causes the plugin's Webpack build to attempt
> processing the Netlify CMS modules with Babel. The modules are massive
> and also prebuilt, so it will (and should) crash your system.

> The PR I raised

fix(gatsby-plugin-netlify-cms): exclude node_modules from cms builds
gatsbyjs/gatsby#15191

> just adds that exclusion back in to the plugin's Webpack config.
> Please don't override your node max file size limit to fix this!

feat(gatsby): enable babel-loader for all dependencies
gatsbyjs/gatsby#14111
ChristopherBiscardi added a commit to ChristopherBiscardi/gatsby that referenced this pull request Jul 1, 2019
Because of gatsbyjs#14111, we can remove the custom theme processing.
johno pushed a commit to johno/gatsby that referenced this pull request Jul 17, 2019
@vladar vladar mentioned this pull request Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
OSS Roadmap
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants