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

fix(babel-preset-gatsby): remove spread operator from node builds #29346

Merged
merged 3 commits into from
Feb 5, 2021

Conversation

wardpeet
Copy link
Contributor

@wardpeet wardpeet commented Feb 5, 2021

Description

babel/babel#12722 Introduced a bug in Gatsby where ssr could not be built. We disabled spread operator in preset-env and added it manually to support non loose mode. Spread polyfilling is not necessary in node > 5 so disabling that fixes our issues.

Related Issues

Fixes #29326

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Feb 5, 2021
@wardpeet wardpeet added status: needs core review Currently awaiting review from Core team member topic: webpack/babel Webpack or babel and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Feb 5, 2021
ascorbic
ascorbic previously approved these changes Feb 5, 2021
@@ -114,7 +119,7 @@ export default function preset(_, options = {}) {
// absoluteRuntime: absoluteRuntimePath,
},
],
[
isBrowser && [
Copy link

Choose a reason for hiding this comment

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

Will class-transform be provided when isBrowser is true? If not, Babel will still throw on super(...args). Before 7.12.11, babel outputs untranspiled super(...args) without any warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will work unless people have a custom browserslist where spread is not needed 🤔. Good call

Copy link

Choose a reason for hiding this comment

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

Consider switch to assumptions after Babel 7.13 is released. Ideally what we need is if you enable spread-transform because of your specified targets, do not assume "iterable is array", which what the loose mode of spread-transform did. Here we are unconditionally transpiling ... even if the target already supports 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.

Awesome! Didn't know about assumptions yet. That will fix it. In the next major I'll add loose as a parameter so people can configure preset-env so we don't have to add a separate module for spread.

isBrowser && [
resolve(`@babel/plugin-transform-spread`),
{
loose: false, // Fixes #14848
},
],
isBrowser && [
Copy link

@JLHwung JLHwung Feb 5, 2021

Choose a reason for hiding this comment

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

Instead of transpiling both class and spread unconditionally, consider check if transform-spread is actually required for the given targets:

import { isRequired } from "@babel/helper-compilation-targets";

isRequired("transform-spread", targets)
// returns boolean, whether `transform-spread` is required.
// see https://unpkg.com/browse/@babel/compat-data@7.12.13/data/plugins.json for available feature names

You can plug in transform-classes safely because if a target does not support ... natively, it does not support class either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL @babel/helper-compilation-targets. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@babel/helper-compilation-targets didn't accept targets how we handle it and pass it on to preset-env so I have to do some more digging and make it more resilient. I've made a ticket in our issue tracker

Copy link

Choose a reason for hiding this comment

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

@babel/preset-env uses isRequire under the hood: https://github.com/babel/babel/blob/74ed698c2e2140c0ef685b1542ef6ad8f20f0b20/packages/babel-preset-env/src/index.js#L288

It seems to me a bug of @babel/helper-compilation-targets if it didn't accept the targets passed to preset-env. Please file an issue to Babel if you can isolate the bug. Thanks!

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Let's ship it.

@babel/helper-compilation-targets seems to be more involved than initially seemed from my offline chat with @wardpeet, so that will have to be done separately

@pieh pieh merged commit d163691 into master Feb 5, 2021
@pieh pieh deleted the fix/spread-issue branch February 5, 2021 17:29
ascorbic pushed a commit that referenced this pull request Feb 5, 2021
…9346)

* fix(babel-preset-gatsby): remove spread operator from node builds

* fix transform-classes

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
(cherry picked from commit d163691)
pieh pushed a commit that referenced this pull request Feb 5, 2021
…9346) (#29357)

* fix(babel-preset-gatsby): remove spread operator from node builds

* fix transform-classes

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
(cherry picked from commit d163691)

Co-authored-by: Ward Peeters <ward@coding-tech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs core review Currently awaiting review from Core team member topic: webpack/babel Webpack or babel
Projects
None yet
4 participants