-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Add babel-preset-gatsby #8724
Add babel-preset-gatsby #8724
Conversation
This PR adds `babel-preset-gatsby`, a new package that exposes our babel config as a handy preset that we can re-use in all our internal packages as well as in public packages. I've added a new option, `targets`, to overwrite the preset-env targets. You can see the changes in the updated Babel documentation. In gatsbyjs#6170, @DSchau mentioned that we want to replace [`babel-loader-helpers`][] with this preset as well (eventually). I'm not really sure what this helper is doing right now so I decided to not touch it. It seems like the helper is used for users of Gatsby. I'm thinking now that my changes to the Babel documentation article should not be made until we also upgrade the above helper to use our preset (e.g. the helper adds `babel-plugin-macros` which is not used in our internal preset). Let me know if I should revert my changes to the article. There are a few other guides that explain a custom babel config. Should I update them as well (test-css-in-js and unit-testing)? [`babel-loader-helpers`]: https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/utils/babel-loader-helpers.js
@@ -0,0 +1,15 @@ | |||
{ | |||
"name": "babel-preset-gatsby", | |||
"version": "2.5.0", |
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.
Not really sure what to use here. I copied from babel-plugin-remove-graphql-queries
. Maybe we want to add a changelog as well?
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.
Nope! Don't add a changelog, it'll get autogenerated by Lerna I believe.
cc @pieh sanity check me on this one :)
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.
let's start with 1.0.0 (or lower heh) - all our packages have independent versions
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.
OK awesome! First of all; thanks so much!
I left a number of comments that I hope should clarify what we're looking for here, and help get this ship shape.
In general, I think there were two different approaches here:
- There's a gatsby preset for gatsby packages (still helpful!)
- There's a gatsby preset for gatsby sites, and this is where I was hoping we'd go!
It's probably my fault for not clarifying more in that issue, but I think this is still a great PR we can tweak to get merged. Note: that I do think there's value in adding both here, I'm just not sure how to best do it yet. babel-preset-gatsby
(for sites) and babel-preset-gatsby-package
(for packages?)
@@ -1,5 +1,5 @@ | |||
{ | |||
"presets": [ | |||
["../../.babel-preset.js"] | |||
["gatsby"] |
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.
OK so I thought we were on the same page here, but this seems weird. I wasn't thinking we'd use the gatsby
preset for our own packages, but rather we'd expose this so that other gatsby sites could use this in e.g. jest tests or as a base that could then be extended.
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.
Right... This makes a lot more sense now. 🤕
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.
💯% my fault in the issue. I can totally see how it could be interpreted both ways!
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.
Should we still use the same preset for our internal packages? I find it confusing that we have two different presets apparently 🙂
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.
What about babel-preset-gatsby
(for sites using gatsby) and babel-preset-gatsby-package
(for packages/utilities for gatsby)
Also open to opinions 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.
I was more thinking about unifying those presets - Not sure if that's possible though. 🙂 Before we start adding an option { package: true }
it's probably better to separate those.
Our packages seem to use:
@babel/preset-env
@babel/preset-react
@babel/preset-flow
@babel/plugin-proposal-class-properties
@babel/plugin-proposal-optional-chaining
@babel/plugin-transform-runtime
For Jest config of sites we seem to suggest:
const babelOptions = {
presets: ["@babel/react", "@babel/env"],
plugins: [
"@babel/plugin-proposal-optional-chaining",
"@babel/plugin-proposal-class-properties",
],
}
Which can probably be the same as the one for packages without the browser stuff.
For sites (the thing we generate with babel-loader-helpers) the thing seems to be a lot more complex, involving different stages and something referred to as required/fallback plugins). In the Babel documentation article we mention this config:
{
presets: [
[
"@babel/preset-env",
{
loose: true,
modules: false,
useBuiltIns: "usage",
shippedProposals: true,
targets: {
browsers: [">0.25%", "not dead"],
},
},
],
[
"@babel/preset-react",
{
useBuiltIns: true,
pragma: "React.createElement",
},
],
],
plugins: [
[
"@babel/plugin-proposal-class-properties",
{
loose: true,
},
],
"@babel/plugin-syntax-dynamic-import",
"babel-plugin-macros",
[
"@babel/plugin-transform-runtime",
{
helpers: true,
regenerator: true,
},
],
],
}
Now I understand why you were talking about updating the helper in a follow-up 🙈
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 think it makes sense to remove the fallbackPlugins
/fallbackPresets
from the babel-loader in favor of the babel-preset-gatsby
which should be the only fallback for sites. This means that we need:
- @babel/preset-env
- @babel/preset-react
- @babel/plugin-proposal-class-properties
- babel-plugin-macros
- @babel/plugin-syntax-dynamic-import
- @babel/plugin-transform-runtime
We probably don't want to add babel-plugin-macros
and @babel/plugin-syntax-dynamic-import
for our packages and also don't want to add @babel/preset-flow
and @babel/plugin-proposal-optional-chaining
to our sites. So I guess we should still separate for now or do you have an idea how we can unify this?
@@ -0,0 +1,15 @@ | |||
{ | |||
"name": "babel-preset-gatsby", | |||
"version": "2.5.0", |
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.
Should be 1.0.0?
@philipp-spiess this is great! BTW, will it allow for implementing #2114? |
@KyleAMathews I don't think this PR changes anything in that regard. We'd still need a flag/env-var to turn on "modern"-mode. I'm not finding a lot of resources how such a webpack setup could look like so it's hard to tell. |
docs/docs/babel.md
Outdated
@@ -22,48 +22,26 @@ browsers. | |||
## How to use a custom .babelrc file | |||
|
|||
Gatsby ships with a default .babelrc setup that should work for most sites. If you'd like | |||
to add custom Babel presets or plugins, we recommend copying our default .babelrc below | |||
to the root of your site and modifying it per your needs. | |||
to add custom Babel presets or plugins, you can import our preset and overwrite the target option. |
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.
does "our preset" mean babel-preset-gatsby
? If so, it would be clearer to just say that instead of "our preset."
@@ -22,48 +22,26 @@ browsers. | |||
## How to use a custom .babelrc file | |||
|
|||
Gatsby ships with a default .babelrc setup that should work for most sites. If you'd like |
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.
Is the default setup the babel-preset-gatsby-package
thing? If so, would it make sense to link to it here and name it?
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.
babel-preset-gatsby
would/will be the one used by 99% of users!
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 have yet to finish the implementation as well as the docs. Will definitely incorporate this, thanks 🙂
My plan is to have it ready for a next review cycle until the end of the week.
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.
@philipp-spiess whenever you can get to is great! I think this is an impactful change, particularly in re: to being easier to set up testing!
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 approve the docs changes as long as this is technically good to go, which others will have to decide 👍
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.
Alright @DSchau this should be good for a second review pass 🙂 Thanks for your patience.
} | ||
} else { | ||
targets = { | ||
browsers: pluginBabelConfig.browserslist, |
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 a bit confused here about pluginBabelConfig.browserslist
. Where are we reading pluginBabelConfig
from? There seems to be a ./.cache/babelState.json
file but I don't think I can access this from our new npm package.
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.
See #8724 (comment)
} | ||
} else { | ||
targets = { | ||
browsers: [`>0.25%`, `not dead`], |
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've hardcoded this now. We need to figure out how we can retrieve that from ./.cache/babelState.json
(see my comment on the babel-loader-helpers.js
file).
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.
Still looking into that. Seems like the file is generated here:
gatsby/packages/gatsby/src/internal-plugins/load-babel-config/gatsby-node.js
Lines 8 to 28 in f7d3231
exports.onPreBootstrap = async ({ store }) => { | |
const { directory, browserslist } = store.getState().program | |
const directoryPath = withBasePath(directory) | |
await apiRunnerNode(`onCreateBabelConfig`, { | |
stage: `develop`, | |
}) | |
await apiRunnerNode(`onCreateBabelConfig`, { | |
stage: `develop-html`, | |
}) | |
await apiRunnerNode(`onCreateBabelConfig`, { | |
stage: `build-javascript`, | |
}) | |
await apiRunnerNode(`onCreateBabelConfig`, { | |
stage: `build-html`, | |
}) | |
const babelrcState = store.getState().babelrc | |
babelrcState.browserslist = browserslist | |
const babelState = JSON.stringify(babelrcState.stages, null, 4) | |
await fs.writeFile(directoryPath(`.cache/babelState.json`), babelState) | |
} |
I understand this as being placed inside the .cache
directory inside the site root, correct?
In this case I can probably just use the same pattern to access this file like we currently do:
gatsby/packages/gatsby/src/utils/babel-loader-helpers.js
Lines 4 to 13 in f7d3231
const loadCachedConfig = () => { | |
let pluginBabelConfig = { test: { plugins: [], presets: [] } } | |
if (process.env.NODE_ENV !== `test`) { | |
pluginBabelConfig = require(path.join( | |
process.cwd(), | |
`./.cache/babelState.json` | |
)) | |
} | |
return pluginBabelConfig | |
} |
Which is what I'm doing right now.
# Conflicts: # yarn.lock
00be903
to
637529a
Compare
@philipp-spiess circling back to this; sorry for the delay! This is looking great. Would you mind merging upstream/master into your fork? We made some fixes to the Windows pipeline, so the failures would be a little clearer with that change. In looking through the Azure pipeline stuff, it looks like some tests are failing with the path separator character. Maybe instead of expect.stringContaining(`@babel/preset-env`); we use const path = require(`path`) // if it's not already required
expect.stringContaining([`@babel`, `preset-env`].join(path.sep)) that seems to be a decent solution but open to any and all solutions! |
@DSchau You can use const path = require(`path`) // if it's not already required
- expect.stringContaining([`@babel`, `preset-env`].join(path.sep))
+ expect.stringContaining(path.join(`@babel`, `preset-env`)) Not sure if there's a better way. Cross-compatibility is hard. |
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 working on this! ❤️ Btw, you have excellent documentation skills. 👌
Adding a .babelrc
file reverts all Babel config changes made by Gatsby plugins, right? Let's say that I want to add Flow, if I have a .babelrc
file, gatsby-plugin-flow will no longer work and I should add @babel/preset-flow
to .babelrc
file myself, right? If yes, maybe it would be useful to document that as well.
|
||
### `targets` | ||
|
||
`{ [string]: number | string }`, defaults to `{ "browsers": ["last 4 versions", "safari >= 7", "ie >= 9"] }` in production and `{ "browsers": ["last 2 versions", "not ie <= 11", "not android 4.4.3"] }` in development when targeting the browser and `{ "node": 6 }` in production and `{ "node": "current" }` in development when targeting Node.js. |
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.
Is it possible for users to configure this by add a .browserslistrc
file? That way PostCSS can pick it up as well, which will result in a clean support table for the project.
If yes, it would be great to add this to the docs as well.
EDIT: I found out that Gatsby supports Browserslist, maybe that's where that pluginBabelConfig.browserslist
comes from that you were wondering about.
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.
Riiight that's documented here: https://www.gatsbyjs.org/docs/browser-support/#specify-what-browsers-your-project-supports-using-browserslist
In this case I don't think it's useful to mention this as a use case for the custom babel config like we do right now and add the targets
option to babel-preset-gatsby
at all.
I'm trying to think of another use case for customizing the Babel setup but it kinda feels plugins are already doing most of the "common" stuff. Maybe we can show how to add a hypothetical babel-transform-awesome-feature
🙈 ?
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 I think it's out of scope for this PR, you've done so much already and this uncertainty isn't related to your PR. It's best to clarify this stuff in another PR.
@tricoder42 that seems like a better solution, thank you for the suggestion! |
@silvenon I think this still works. We're always using a Gatsby controlled Babel config which always added a couple of "fallback" transforms if no Custom plugins can still add additional babel presets like they do right now: https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-plugin-flow/src/gatsby-node.js |
Thanks, I did a little testing in the meantime and it turns out that a custom Babel configuration is a starting point for all modifications by Gatsby plugins, which is a relief. I will make this clear in a documentation PR later on. |
Since this is getting a bit confusing (at least for me), here are the two open questions: The rest should be resolved by now. 🙂 |
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.
Gonna check this out locally, but I think this is good to go and seriously a huge help for a lot of people, so thanks so much!
docs/docs/babel.md
Outdated
], | ||
} | ||
``` | ||
|
||
For more advanced configurations, you can also copy the defaults from [`babel-preset-gatsby`](https://github.com/gatsbyjs/gatsby/tree/master/packages/babel-preset-gatsby) and customize them to suite your needs. |
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.
For more advanced configurations, you can also copy the defaults from [`babel-preset-gatsby`](https://github.com/gatsbyjs/gatsby/tree/master/packages/babel-preset-gatsby) and customize them to suite your needs. | |
For more advanced configurations, you can also copy the defaults from [`babel-preset-gatsby`](https://github.com/gatsbyjs/gatsby/tree/master/packages/babel-preset-gatsby) and customize them to suit your needs. |
But not important enough to prevent merging :) Working through this now!
@@ -22,7 +22,7 @@ First you need to install Jest and some more required packages. You need to | |||
install Babel 7 as it's required by Jest. | |||
|
|||
```sh | |||
npm install --save-dev jest babel-jest react-test-renderer identity-obj-proxy 'babel-core@^7.0.0-0' @babel/core @babel/preset-env @babel/preset-react @babel/plugin-proposal-class-properties @babel/plugin-proposal-optional-chaining | |||
npm install --save-dev jest babel-jest react-test-renderer identity-obj-proxy 'babel-core@^7.0.0-0' @babel/core babel-preset-gatsby |
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.
This is so much better 💯
|
||
expect(presets).toEqual([ | ||
[ | ||
expect.stringContaining(path.join(`@babel`, `preset-env`)), |
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.
Love these tests, a lot. Thanks for adding!
], | ||
expect.stringContaining(path.join(`@babel`, `preset-flow`)), | ||
]) | ||
expect(plugins).toEqual([ |
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.
@pieh unrelated, but I think these are a little more useful/readable than snapshots in most cases, so I think this is something we could adopt going forward.
Holy buckets, @philipp-spiess — we just merged your PR to Gatsby! 💪💜 Gatsby is built by awesome people like you. Let us say “thanks” in two ways:
If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’. Thanks again! |
@philipp-spiess thanks again for this PR, like I mentioned, I think it's hugely impactful and it tends to make sense to encapsulate this internal tooling into packages! Sorry for the delay in getting it merged, but glad we finally did :) Releasing a bunch of packages now 😅 |
This reverts commit 69faca0.
Part of gatsbyjs#6170 This PR adds `babel-preset-gatsby`, a new package that exposes our babel config as a handy preset that we can re-use in all our internal packages as well as in public packages. I've added a new option, `targets`, to overwrite the preset-env targets. You can see the changes in the updated Babel documentation. In gatsbyjs#6170, @DSchau mentioned that we want to replace [`babel-loader-helpers`][] with this preset as well (eventually). I'm not really sure what this helper is doing right now so I decided to not touch it. It seems like the helper is used for users of Gatsby. I'm thinking now that my changes to the Babel documentation article should not be made until we also upgrade the above helper to use our preset (e.g. the helper adds `babel-plugin-macros` which is not used in our internal preset). Let me know if I should revert my changes to the article. There are a few other guides that explain a custom babel config. Should I update them as well (test-css-in-js and unit-testing)? [`babel-loader-helpers`]: https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/utils/babel-loader-helpers.js
…rst (gatsbyjs#9322) * Revert "Add babel-preset-gatsby (gatsbyjs#8724)" This reverts commit 69faca0. * chore: add these packages _first_ then use them in a future PR
Part of gatsbyjs#6170 This PR adds `babel-preset-gatsby`, a new package that exposes our babel config as a handy preset that we can re-use in all our internal packages as well as in public packages. I've added a new option, `targets`, to overwrite the preset-env targets. You can see the changes in the updated Babel documentation. In gatsbyjs#6170, @DSchau mentioned that we want to replace [`babel-loader-helpers`][] with this preset as well (eventually). I'm not really sure what this helper is doing right now so I decided to not touch it. It seems like the helper is used for users of Gatsby. I'm thinking now that my changes to the Babel documentation article should not be made until we also upgrade the above helper to use our preset (e.g. the helper adds `babel-plugin-macros` which is not used in our internal preset). Let me know if I should revert my changes to the article. There are a few other guides that explain a custom babel config. Should I update them as well (test-css-in-js and unit-testing)? [`babel-loader-helpers`]: https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/utils/babel-loader-helpers.js
…rst (gatsbyjs#9322) * Revert "Add babel-preset-gatsby (gatsbyjs#8724)" This reverts commit 69faca0. * chore: add these packages _first_ then use them in a future PR
Part of #6170
This PR adds
babel-preset-gatsby
, a new package that exposes our babel config as a handy preset that we can re-use in all our internal packages as well as in public packages.I've added a new option,
targets
, to overwrite the preset-env targets. You can see the changes in the updated Babel documentation.In #6170, @DSchau mentioned that we want to replace
babel-loader-helpers
with this preset as well (eventually). I'm not really sure what this helper is doing right now so I decided to not touch it. It seems like the helper is used for users of Gatsby.I'm thinking now that my changes to the Babel documentation article should not be made until we also upgrade the above helper to use our preset (e.g. the helper adds
babel-plugin-macros
which is not used in our internal preset). Let me know if I should revert my changes to the article.There are a few other guides that explain a custom babel config. Should I update them as well (test-css-in-js and unit-testing)?