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

Add babel-preset-gatsby #8724

Merged
merged 23 commits into from
Oct 23, 2018
Merged

Conversation

philipp-spiess
Copy link
Contributor

@philipp-spiess philipp-spiess commented Oct 2, 2018

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)?

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
@philipp-spiess philipp-spiess requested a review from a team as a code owner October 2, 2018 21:38
@philipp-spiess philipp-spiess requested a review from a team October 2, 2018 21:38
@@ -0,0 +1,15 @@
{
"name": "babel-preset-gatsby",
"version": "2.5.0",
Copy link
Contributor Author

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?

Copy link
Contributor

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 :)

Copy link
Contributor

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

Copy link
Contributor

@DSchau DSchau left a 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:

  1. There's a gatsby preset for gatsby packages (still helpful!)
  2. 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?)

.babelrc.js Outdated Show resolved Hide resolved
@@ -1,5 +1,5 @@
{
"presets": [
["../../.babel-preset.js"]
["gatsby"]
Copy link
Contributor

@DSchau DSchau Oct 2, 2018

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.

Copy link
Contributor Author

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. 🤕

Copy link
Contributor

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!

Copy link
Contributor Author

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 🙂

Copy link
Contributor

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 🤷‍♂️

Copy link
Contributor Author

@philipp-spiess philipp-spiess Oct 2, 2018

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:

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 🙈

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 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?

packages/babel-preset-gatsby/README.md Outdated Show resolved Hide resolved
packages/babel-preset-gatsby/package.json Outdated Show resolved Hide resolved
@@ -0,0 +1,15 @@
{
"name": "babel-preset-gatsby",
"version": "2.5.0",
Copy link
Contributor

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?

packages/babel-preset-gatsby/package.json Outdated Show resolved Hide resolved
packages/babel-preset-gatsby/src/__tests__/index.js Outdated Show resolved Hide resolved
@KyleAMathews
Copy link
Contributor

@philipp-spiess this is great! BTW, will it allow for implementing #2114?

@philipp-spiess
Copy link
Contributor Author

@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.

@@ -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.
Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Contributor

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!

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 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.

Copy link
Contributor

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!

Copy link
Contributor

@shannonbux shannonbux left a 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 👍

Copy link
Contributor Author

@philipp-spiess philipp-spiess left a 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.

packages/babel-preset-gatsby/package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
packages/babel-preset-gatsby/src/index.js Show resolved Hide resolved
}
} else {
targets = {
browsers: pluginBabelConfig.browserslist,
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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}
} else {
targets = {
browsers: [`>0.25%`, `not dead`],
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'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).

Copy link
Contributor Author

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:

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:

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.

packages/gatsby/package.json Show resolved Hide resolved
packages/gatsby/package.json Show resolved Hide resolved
@DSchau
Copy link
Contributor

DSchau commented Oct 10, 2018

@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!

@tricoder42
Copy link

@DSchau You can use path.join directly:

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.

Copy link

@silvenon silvenon left a 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.

docs/docs/babel.md Outdated Show resolved Hide resolved
docs/docs/unit-testing.md Show resolved Hide resolved

### `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.
Copy link

@silvenon silvenon Oct 13, 2018

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.

Copy link
Contributor Author

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 🙈 ?

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.

@DSchau
Copy link
Contributor

DSchau commented Oct 13, 2018

@tricoder42 that seems like a better solution, thank you for the suggestion!

@philipp-spiess
Copy link
Contributor Author

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.

@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 .babelrc was found. The change I'm is only changing these "fallback" transforms to be a single preset instead.

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

@silvenon
Copy link

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.

@philipp-spiess
Copy link
Contributor Author

philipp-spiess commented Oct 14, 2018

Since this is getting a bit confusing (at least for me), here are the two open questions:

  1. Add babel-preset-gatsby #8724 (comment)
  2. Add babel-preset-gatsby #8724 (comment)

The rest should be resolved by now. 🙂

Copy link
Contributor

@DSchau DSchau left a 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!

],
}
```

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.
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
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
Copy link
Contributor

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`)),
Copy link
Contributor

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([
Copy link
Contributor

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.

@DSchau DSchau merged commit 69faca0 into gatsbyjs:master Oct 23, 2018
@gatsbot
Copy link

gatsbot bot commented Oct 23, 2018

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:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

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!

@DSchau
Copy link
Contributor

DSchau commented Oct 23, 2018

@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 😅

DSchau added a commit to DSchau/gatsby that referenced this pull request Oct 23, 2018
DSchau added a commit that referenced this pull request Oct 23, 2018
…rst (#9322)

* Revert "Add babel-preset-gatsby (#8724)"

This reverts commit 69faca0.

* chore: add these packages _first_ then use them in a future PR
@philipp-spiess philipp-spiess deleted the babel-preset-gatsby branch October 23, 2018 19:53
gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
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
gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
…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
mwfrost pushed a commit to mwfrost/gatsby that referenced this pull request Apr 20, 2023
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
mwfrost pushed a commit to mwfrost/gatsby that referenced this pull request Apr 20, 2023
…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
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.

8 participants