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

Babel 7 #1955

Merged
merged 46 commits into from
Sep 16, 2018
Merged

Babel 7 #1955

merged 46 commits into from
Sep 16, 2018

Conversation

devongovett
Copy link
Member

@devongovett devongovett commented Aug 29, 2018

This updates to use Babel 7 internally. Babel 6 will also be supported for projects that haven't upgraded yet. Fixes #868.

To do:

  • Support both Babel 6 and 7 at the same time.
  • Automatically install babel-core v6 in a project if we detect babel 6 plugins.
  • Load configs using Babel's helper methods
  • Use babel-preset-env's new polyfill useBuiltIns: "usage" option.
  • Ensure all existing tests pass.
  • Add tests to ensure babel 6 is still supported.

If anyone wants to help with any of these items, please make PRs against the babel7 branch. Thanks! 🙏

// from babel-types. remove when we upgrade to babel 7.
// https://github.com/babel/babel/blob/0189b387026c35472dccf45d14d58312d249f799/packages/babel-types/src/index.js#L347
// from @babel/types. remove when we upgrade to babel 7.
// https://github.com/babel/babel/blob/0189b387026c35472dccf45d14d58312d249f799/packages/@babel/types/src/index.js#L347
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: remove this file now that we are on babel 7 :)

@devongovett devongovett mentioned this pull request Aug 29, 2018
@tunnckoCore
Copy link

I think we should have (as deps here) both Babel 6 and Babel 7 pacakges. Then detect if the cwd's package.json has babel- prefixed deps/devDeps, then use the v6 stack, otherwise use the v7 things.

I'm not sure if there is AST changes between v6 and v7? If not, then that would be great! Otherwise, that would be the most tricky and hard part. The rest is easy.

* Fix babel-types dependency name

* Upgrade to Babel 7
@devongovett
Copy link
Member Author

I believe there may be AST changes, not 100% sure though. cc. @hzoo @loganfsmyth

I was going to use babel 7 for all our internal parsing needs. Many parcel projects don't need any custom babel plugins at all since we generate a config using preset-env and preset-react already. But for those that do have a .babelrc, I was going to try to detect the versions of the plugins (from package.json) and if they are v6, then install babel-core v6 locally in the project and use that to run the custom transforms. If the ASTs aren't compatible, we'll stringify with babel 7 and reparse with babel 6.

@hzoo
Copy link

hzoo commented Aug 29, 2018

👋 There are a few AST changes but it's small https://babeljs.io/docs/en/v7-migration-api#ast-changes, mostly to match estree https://babeljs.io/docs/en/v7-migration-api#removal

@devongovett
Copy link
Member Author

The AST changes look pretty minimal. I wonder if it would be faster to traverse and convert the AST to a babel 6 compatible one and back than to generate + parse + generate + parse. Should be pretty easy to write a converter.

@hzoo
Copy link

hzoo commented Aug 29, 2018

That would be essentially what babel-eslint does (parse with babel and then modify it to support espree/eslint), except it's for v6. https://github.com/babel/babel-eslint/blob/6aa8b6f02ff83cfb14eae2432f226f113929a50f/lib/parse.js#L90 Ideally we would just output a v6 compat mode so it would be faster but yeah not sure how there would be a good way of doing this.

@tunnckoCore
Copy link

tunnckoCore commented Aug 29, 2018

Yea, 1) parse with Babel 7 always, then 2) convert them back to v6, 3) do the transofrms, 4) covert them back to v7 and use v7's stringify. Because changes are really minimal. Rest would be just the same.

I'm not very familiar with the codebase though, but just thinking. ;d ;]

package.json Outdated
"@babel/preset-env": "^7.0.0",
"@babel/template": "^7.0.0",
"@babel/traverse": "^7.0.0",
"@babel/types": "^7.0.0",
"ansi-to-html": "^0.6.4",
"babel-code-frame": "^6.26.0",
"babel-core": "^6.25.0",
Copy link
Member

@DeMoorJasper DeMoorJasper Aug 29, 2018

Choose a reason for hiding this comment

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

Shouldn't this be localRequired as we'll use Babel 7 for everything internal?

(Or a devDep if we'll test this)

Copy link
Member

Choose a reason for hiding this comment

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

WIP for this: #1958

@devongovett devongovett changed the title WIP: babel 7 Babel 7 Sep 16, 2018
@devongovett devongovett merged commit c9c5805 into master Sep 16, 2018
@devongovett devongovett deleted the babel7 branch September 16, 2018 05:59
@devongovett
Copy link
Member Author

Merged and published as a beta in v1.10.0-beta.0. Please help test this out and report any bugs you find! 🎉

@tunnckoCore
Copy link

Sweeet. So, in a new project it just works, right?

@bkniffler
Copy link

I just switched to v1.10.0-beta.0 and except for updating the babelrc file to use the later @babel plugins, everything works good.

@loganfsmyth
Copy link

@devongovett Cool, I'll respond to your issue when I get some time. I think some of they are reasonable requests.

I think my fundamental issue with your approach here is that it assumes that it is even possible to autodetect the version of Babel in use. While that may be try between 6 and 7 in most cases, that doesn't seem like a thing we can rely on long-term when Babel releases further major versions. Along with that, there's no guarantee that, as users migrate to JS-based config files, that config files will be parsable for metadata in the way you're doing now. Especially if tools start adopting things like Yarn's new proposed resolution logic, I'd expect that all config files will transition to requireing plugins first, meaning that their aren't any paths available for you to resolve anyway, at runtime, so it seems like the vast amount of the work you'd need to do to detect the version would have to be performed as static analysis on the config files, without even running them, and that's not something Babel can help you with.

I'm 100% behind Parcel's desire to be zero-config, but it seems like you want to be both zero-config and un-opinionated, and those seem directly contradictory. I think you should just update to use Babel 7 as a new major bump and drop Babel 6, and bump your major any time any new dependency version comes out.

@devongovett
Copy link
Member Author

devongovett commented Sep 16, 2018

@bkniffler

except for updating the babelrc file to use the later @babel plugins, everything works good.

Does this mean something is broken using newer babel?

@loganfsmyth

While that may be try between 6 and 7 in most cases, that doesn't seem like a thing we can rely on long-term when Babel releases further major versions.

One of the things mentioned as part of the babel 7 upgrade was that all of the plugins and presets now have a peer dependency on the version of @babel/core that they need. This makes it pretty easy for us to auto detect the version of babel that Parcel needs to use. I think babel should enforce this requirement on third party plugins as well.

You're right that JS-based config files are problematic, and I think they are not really a great idea in general. Parcel will support them, but probably not quite as automatically as .babelrc. We cannot help auto install plugins, or detect the version of babel to use automatically in that case. I think if people want to use JS configs, then they are responsible for ensuring that all of their plugins are installed themselves, and that they have a dependency on babel core in their package.json prior to running Parcel.

@devongovett
Copy link
Member Author

Also, @loganfsmyth @hzoo is the AST converter we wrote for babel 6 -> 7 and visa versa something that you think should exist in babel itself? I could see it being useful for other projects, and probably not something we should be maintaining in Parcel ideally.

useBuiltIns: useBuiltIns ? 'entry' : false,
shippedProposals: true
}
).plugins;

Choose a reason for hiding this comment

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

FYI, presets can have any of Babel's standard options, so you're throwing those away here. That means if we refactor this in the future to have other things, things will break in Parcel.


module.exports = getBabelConfig;

function mergeConfigs(result, config) {

Choose a reason for hiding this comment

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

This merge appears to just discard any other options that were specified in the Babel config.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a very simple merge, yes. The first config to be passed is the user specified one. The others are all internally generated, and only have plugins and presets, so that shouldn't be a problem.

Choose a reason for hiding this comment

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

Fair point. I guess this more goes back to my other comment then. preset-env could at any time start using overrides or env or test/include/exclude, or add other options as top-level flags, and then it would cease to function on Parcel because Parcel is hard-coded to only carry over plugins.

);
}

for (let key of ['extends', 'overrides', 'test', 'include', 'exclude']) {

Choose a reason for hiding this comment

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

I think you'll want 'env' in here too.

config.configFile = false;
config.parserOpts = Object.assign({}, config.parserOpts, {
allowReturnOutsideFunction: true,
strictMode: false,

Choose a reason for hiding this comment

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

Assuming this is to allow parsing both scripts and modules, you'd likely be better off doing sourceType: "unambiguous".

}

let plugins = presetEnv.default(
{assertVersion: () => true},

Choose a reason for hiding this comment

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

Could you point me to where you are processing these plugins? I'm having trouble finding it.

Part of my issue here is that the exact return value of a preset is not a public API, because a preset can also have other presets, and what we'd actually need is an API to fully flatten the configuration into a list of plugins, which is pretty much what loadOptions already provides. We wouldn't want to expose makeAPI because it exposes internal implementation details of Babel itself, and it's not obvious what anyone would actually be able to do with the result anyway, other than pass it right back to Babel again unprocessed.

@loganfsmyth
Copy link

One of the things mentioned as part of the babel 7 upgrade was that all of the plugins and presets now have a peer dependency on the version of @babel/core that they need. This makes it pretty easy for us to auto detect the version of babel that Parcel needs to use. I think babel should enforce this requirement on third party plugins as well.

I actually have very mixed feelings about our addition of peerDependencies and I think it yet to be seem whether they will be good or bad for us. Take even just the Parcel case, if the user installs Babel plugins in the project root and installs @babel/core, there's no guarantee that the version there will be the version that Parcel itself runs, since it might have it's own copy in node_modules/parcel/node_modules/@babel/core, meaning that the Babel version loaded by peerDependencies for the user's plugin would be different from the version that is actually calling Babel. That'd be different if Parcel itself wanted to require users to install @babel/core and itself had a peerDependency on @babel/core, but that's not happening. We've already got static assertions inside our own plugins either way, so in the end I'm not sure the peerDependencies in the plugins actually get us anything.

You're right that JS-based config files are problematic, and I think they are not really a great idea in general. Parcel will support them, but probably not quite as automatically as .babelrc. We cannot help auto install plugins, or detect the version of babel to use automatically in that case. I think if people want to use JS configs, then they are responsible for ensuring that all of their plugins are installed themselves, and that they have a dependency on babel core in their package.json prior to running Parcel.

I understand where you're coming from, but I think for the average user, the expressive abilities of JS configs usually end up outweighing any benefits of configs being static files. If the user's config is representable as static JSON, you'll still get static JSON back from loadPartialConfig, so the benefit of static files is really only being able to process them without calling Babel itself, and I think that's just going to be a losing battle all around.

@devongovett
Copy link
Member Author

Parcel never calls user-specified plugins with its internally installed copy of @babel/core. It always uses the locally installed one specified by the app, which hopefully would match the peer dependencies of plugins. If one isn't installed locally in the app, we auto install one for the user. See https://github.com/parcel-bundler/parcel/blob/master/src/transforms/babel/babel7.js#L8-L10.

the expressive abilities of JS configs usually end up outweighing any benefits of configs being static files

I disagree. I'm not sure what people are doing that they feel like they need to write a dynamic config with JS, but it seems like that will just be chaos similar to webpack.config.js, gulpfile.js and other past attempts.

That said, as long as there is a way to call into babel to resolve this JS-based config into a static config, it should be fine for Parcel. We won't be able to help the user with auto installation in that case, but that's fine I think. They already opted into advanced-mode by choosing JS config anyway.

@loganfsmyth
Copy link

Parcel never calls user-specified plugins with its internally installed copy of @babel/core

That's fair, but it only means that the reverse problem could exist with Parcel's preset-env since Parcel is the one that depends on preset-env, so the peerDependency will be Parcel's version, even though the Babel transform itself is being run by the user's copy. At the end of the day, it may not matter anyway, but I do think the peer dependency just adds to the confusion.

I disagree. I'm not sure what people are doing that they feel like they need to write a dynamic config with JS, but it seems like that will just be chaos similar to webpack.config.js, gulpfile.js and other past attempts.

I'm sure we could talk about it for a while, but I'm not sure it's really worth it. From my standpoint, I don't consider those chaos.

That said, as long as there is a way to call into babel to resolve this JS-based config into a static config, it should be fine for Parcel. We won't be able to help the user with auto installation in that case, but that's fine I think. They already opted into advanced-mode by choosing JS config anyway.

Yup, totally fair.

@gaui
Copy link

gaui commented Sep 17, 2018

v1.10.0-beta.1 works great - thank you!

@juliedavila
Copy link

@devongovett hmm for me it seems .babelrc and babel.config.js are completely ignored on v1.10.0-beta.1

@gaui
Copy link

gaui commented Sep 17, 2018

@defionscode how so?

@loganfsmyth
Copy link

babel.config.js is definitely ignored at the moment, but .babelrc should work.

@simonlc
Copy link

simonlc commented Sep 19, 2018

I don't understand if .babelrc is fully functional yet, but using the "env" format doesn't work, while .babelrc without it does work.

Doesn't work:

{
  "env": {
    "development": {
      "plugins": [
        "react-hot-loader/babel",
        "@babel/plugin-proposal-class-properties"
      ],
      "presets": [
        "@babel/env",
        "@babel/react"
      ]
    },
    "production": {
      "plugins": [
        ["transform-react-remove-prop-types", {
          "classNameMatchers": ["Component"],
          "removeImport": true
        }],
        "@babel/plugin-proposal-class-properties"
      ],
      "presets": [
        ["@babel/env", {
          "shippedProposals": true
        }],
        "@babel/react"
      ]
    }
  }
}

Works:

{
  "plugins": [
    ["transform-react-remove-prop-types", {
      "classNameMatchers": ["Component"],
      "removeImport": true
    }],
    "@babel/plugin-proposal-class-properties"
  ],
  "presets": [
    ["@babel/env", {
      "shippedProposals": true
    }],
    "@babel/react"
  ]
}

@juliedavila
Copy link

@gaui @loganfsmyth so in particular, it seems that if you supply exclude config option to a babel preset it doesn't actually exclude any plugins. So something like

      presets: [
        [
          '@babel/preset-env',
          {
            exclude: [
              'transform-classes'
            ]
          }
        ]
      ]

I will note that this is the same behavior in the current stable branch with babel < 7. Excludes are seemingly skipped. You can supply any random string to exclude and it will still build no problem when it really should be throwing a babel-related error IF it were reading that section in.

@devongovett devongovett mentioned this pull request Sep 25, 2018
4 tasks
@mririgoyen
Copy link

I'm confused on why this was merged and released without full Babel 7 support? You cannot use babel.config.js as they recommend to do so and, as reported above, using "env" is completely broken in both Babel 6 and 7.

devongovett added a commit that referenced this pull request Oct 15, 2018
devongovett added a commit that referenced this pull request Oct 15, 2018
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.