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

[v2] Babel 7 ideas #3870

Closed
KyleAMathews opened this Issue Feb 6, 2018 · 22 comments

Comments

Projects
5 participants
@KyleAMathews
Contributor

KyleAMathews commented Feb 6, 2018

Chatted with @loganfsmyth this afternoon about how best to upgrade Gatsby to v7 of Babel.

  1. Disable reading any .babelrc files — since we load babelrc directly we shouldn't allow babel to go looking for additional babelrc files. I'm not sure how to do this should would appreciate some pointers.
  2. Remove stage-0 preset
  3. Potentially add way to opt-out of reading a user's .babelrc file — potentially they could be using Babel v6 for e.g. a test framework. While they haven't upgraded that to v7 of Babel, their .babelrc file would cause conflicts. We were unsure if this would be a real issue or not so we'll probably wait and see if there's problems before adding support for this.
  4. Logan says Babel is moving towards having more regular releases. One way to loosen the coupling between Gatsby and Babel is to move Gatsby's Babel setup to a package which Gatsby depends on e.g. gatsby-babel. When there's a new major version of Babel, we release a new version of gatsby-babel that people can optional add as a dependency along with the new version of babel-core. Then when we require gatsby-babel we first check if the site has a direct dependency on gatsby-babel and preferentially use that and fall back on the default version. Once the new version of Babel is stable, on the next major release of Gatsby we'd update our gatsby-babel version.
  5. Copy CRA's implementation for compiling node_modules with babel-preset-env which will fix problems like #3780

/cc @jquense

@jquense

This comment has been minimized.

Collaborator

jquense commented Feb 6, 2018

I think my PR covers 1 and 2 already 👍

I'm not sure about 5, compiling node_modules is likely to dramatically increase build times as well as introduce a lot of unforseen issues for each it solves. I like the concept of it but assuming it's safe or good to recompile dependencies sounds like a recipe for pain. I think CRA can afford these experiments partly because of the audience and because the environment is so tightly controlled, I don't think gatsby fits the same niche

4 is interesting I'd need to think through it a bit more. One thing I'd say now tho is that the relationship between Babel and gatsby users is always gonna be a bit leaky (same with webpack) the only real way to be safe I think is the CRA approach of no config, but we are by default using a consumers Babel config so I don't know :/ it's a hard problem!

@KyleAMathews

This comment has been minimized.

Contributor

KyleAMathews commented Feb 6, 2018

I think my PR covers 1 and 2 already 👍

Oh perfect!

The increase in build time isn't too bad ~20% — which will be more than offset haha by our moving to webpack 3. facebook/create-react-app#3776 (comment)

CRA is being pretty careful about how they do this but yeah, agree it's potentially risky. We can wait until we see how CRA's release goes with this.

Re) 4 — I'm not entirely sold on this — it'd be more work + indirection + Babel isn't going to be doing that many releases. As long as our release cadence is fairly steady, the wait to get the latest Babel/webpack wouldn't be that long.

@KyleAMathews

This comment has been minimized.

Contributor

KyleAMathews commented Feb 6, 2018

Also re) 4 — this would be an easy refactor we could do down the road when Babel 8 comes out.

@loganfsmyth

This comment has been minimized.

loganfsmyth commented Feb 6, 2018

I don't think 1 is covered. It needs to set babelrc: false in the config object before passing it off to babel-loader.

Yeah I don't think 4 needs to be a strict blocker after all of our talk today, but it is something that I want on your radar at least.

@KyleAMathews

This comment has been minimized.

Contributor

KyleAMathews commented Feb 10, 2018

Fixed #1 in #3963

@KyleAMathews

This comment has been minimized.

Contributor

KyleAMathews commented Feb 21, 2018

Another benefit of compiling node_modules is we'd automatically get polyfills added for target browsers w/ Babel's super cool new polyfilling magic. E.g React needs Set, Map, and requestAnimationFrame polyfilled for older browsers. But it'd be nice if those were a) automatically added and b) automatically not added if you're targeting newer browsers.

@jquense

This comment has been minimized.

Collaborator

jquense commented Feb 21, 2018

I think we are waiting on: babel/babel#7358 as table-stakes for compiling node-modules if you wanted to follow the issue :)

I wish this was slightly more trodden ground...like what is the cost for all that extra noop parsing for every dependency that doesn't need additional compilation vs the ones that do? Perhaps its fairly safe as a build only optimization? It's probably nice to add something like devBrowsers btw for develop you could speed that up a bunch by only compiling to last version of chrome or something

@KyleAMathews

This comment has been minimized.

Contributor

KyleAMathews commented Feb 21, 2018

CRA disabled loading babelrc for their implementation. Their tests showed build time increase was around ~20% facebook/create-react-app#3776 (comment)

thread-loader is supposed to work well and offset a lot of that though.

Also yeah, great idea to add development babel config that'd only target the latest browsers. Also we could restrict transpiling node_modules to builds perhaps. Though that'd be another production/development inconsistency which would confuse people. With caching though, starting up the dev server should be pretty fast most of the time.

@jquense

This comment has been minimized.

Collaborator

jquense commented Feb 21, 2018

o right we can disable the babelrc per file via the loader, nice when you control the build pipeline :P

@KyleAMathews

This comment has been minimized.

Contributor

KyleAMathews commented Feb 21, 2018

Can be nice! :-D

@hzoo hzoo referenced this issue Feb 21, 2018

Closed

Tracking: Tools that use `@babel/core` as a dep #20

5 of 8 tasks complete
@jbolda

This comment has been minimized.

Contributor

jbolda commented Feb 28, 2018

In consideration of 5), I ran into this when prototyping theme system ideas: #2662 (comment)

The tl;dr, templates in node_modules get weird as they need to be transpiled, but might also contain graphql. If gatsby can transpile, can we also have it extract graphql queries? I'm assuming fragments are just too involved, but extracting from any "components" specified in createPage may be reasonable.

@KyleAMathews

This comment has been minimized.

Contributor

KyleAMathews commented Feb 28, 2018

We'd include our babel plugin which removes the graphql queries.

Supporting queries in node_modules would be great but would be a separate issue. createPage would be an easy start but we'd also probably need to support searching for fragments.

@jbolda

This comment has been minimized.

Contributor

jbolda commented Mar 2, 2018

That sounds perfect to me! I opened an issue for it.

@loganfsmyth

This comment has been minimized.

loganfsmyth commented Mar 4, 2018

I already pinged some of you on babel/babel#7472, which I think will allow Gatsby to avoid reading the .babelrc itself.

I wanted to add that along with that, I'm curious if Gatsby would want to explore using the babel.parse function that we added a week or two ago, but for which I added documentation in that PR.

There's a few places where Gatsby is hardcoded to use Babylon with a specific set of plugins, and using babel.parse would read the user's Babel config to pick up any plugins they might have, so you could avoid hard-coding the list.

@KyleAMathews

This comment has been minimized.

Contributor

KyleAMathews commented Mar 9, 2018

babel.parse sounds great! We could pass in our default babel config as well I assume if the user doesn't have a babelrc?

@loganfsmyth

This comment has been minimized.

loganfsmyth commented Mar 9, 2018

Yeah I think you'd want to. I guess up to you how that might fit in if there are Gatsby plugins enabling other syntax plugins though, since I don't know how easy it would be to trigger your modifyBabelrc handler in those other places you were using Babylon.

@jquense

This comment has been minimized.

Collaborator

jquense commented Mar 16, 2018

@loganfsmyth thanks for all your work on babel/babel#7472 I'm excited to dive into the new apis! One additional thing did occur to me that maybe it doesn't cover. We use babylon directly in a few places (as you've seen) outside of the webpack/babel-loader flow to do graphql query extraction and static analysis. One on going pain point there is that babylon plugins are all opt-in and so we need to keep up with new syntax additions in order to not choke on code using the latest and greatest...not to mention mutually exclusive plugins like flow vs typescript or either decorators. I'd be great if there was an api to get the required parser functions from a local .babelrc. Or maybe the answer there is just to use babel without codegen?

@jquense

This comment has been minimized.

Collaborator

jquense commented Mar 16, 2018

LOL sorry @loganfsmyth I see that you literally answered my question 12 days ago IGNORE ME, sorry for the noise

@loganfsmyth

This comment has been minimized.

loganfsmyth commented Mar 16, 2018

No worries, it's a lot to keep track of :D

@m-allanson m-allanson added this to To Do in Gatsby v2 Release via automation Apr 11, 2018

@loganfsmyth

This comment has been minimized.

loganfsmyth commented Apr 23, 2018

FYI this change will affect Gatsby's builds: facebook/jest#6053 (comment)

@loganfsmyth

This comment has been minimized.

loganfsmyth commented May 17, 2018

Continuing the discussion from before about Gatsby ideally using consistent logic for inject its config, in my opinion Gatsby should use a custom loader using something like the API in use here: zeit/next.js#4417

So in config() you can change the options however you want. To me that says either:

  • You can call the Gatsby plugins inside the config handler, so they get access to the full flattened config
  • You can call the Gatsby plugins before config somehow, but with an empty Babel config, and then merge that into the user's config inside config()
@m-allanson

This comment has been minimized.

Member

m-allanson commented Aug 6, 2018

The remaining work has been moved to other issues, going to close this now. Thanks folks 👍

@m-allanson m-allanson closed this Aug 6, 2018

Gatsby v2 Release automation moved this from To Do - v2 blockers to Done Aug 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment