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

Various updates to Babel 7 branch #1958

Merged
merged 26 commits into from Sep 4, 2018
Merged

Various updates to Babel 7 branch #1958

merged 26 commits into from Sep 4, 2018

Conversation

DeMoorJasper
Copy link
Member

@DeMoorJasper DeMoorJasper commented Aug 29, 2018

  • Remove babel 6 dependencies from pkg.json (Some would be re-added to devDeps for babel6 compatibility tests in the future)
  • Parcel's Babel configs (for Node 6 compatibility) to use Babel 7
  • Update hasPlugin to use arrays to check for both Babel 6 and 7 plugins
  • Remove matches-pattern file and update references to @babel/types.matchesPattern
  • Adds a helper function to detect Babel 6 configs, it's pretty unreliable once you start using a lot of community made babel plugins
  • AST converter Babel 6 <=> Babel 7
  • Add Babel runtime, to decrease parcel's pkg size
  • Parse and Compile Babel 6 assets, postprocess with Babel 7

Items from #1955's todo list implemented in this PR:

  • Support both Babel 6 and 7 at the same time.
  • Automatically install babel-core v6 in a project if we detect babel 6 plugins.
  • Add tests to ensure babel 6 is still supported. (PARTIALLY, should probably have more than 2 tests)

… hasConfig to handle arrays to check babel6 and 7
@DeMoorJasper DeMoorJasper changed the title Various updates to Babel 7 branch WIP: Various updates to Babel 7 branch Aug 29, 2018
@DeMoorJasper DeMoorJasper mentioned this pull request Aug 29, 2018
6 tasks
@DeMoorJasper DeMoorJasper changed the title WIP: Various updates to Babel 7 branch Various updates to Babel 7 branch Aug 29, 2018
@@ -225,12 +225,11 @@ class JSPackager extends Packager {
// Add source map url if a map bundle exists
let mapBundle = this.bundle.siblingBundlesMap.get('map');
if (mapBundle) {
await this.write(
Copy link
Member Author

Choose a reason for hiding this comment

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

Discovered a potential bug with template strings in Babel 7. But I think it has already been reported. Feel free to check if anyone is up for it :)

It might have been an issue in the babel config I wrote, not sure.

@codecov-io
Copy link

codecov-io commented Aug 30, 2018

Codecov Report

❗ No coverage uploaded for pull request base (babel7@e0df562). Click here to learn what that means.
The diff coverage is 90.09%.

Impacted file tree graph

@@            Coverage Diff            @@
##             babel7    #1958   +/-   ##
=========================================
  Coverage          ?   55.54%           
=========================================
  Files             ?       78           
  Lines             ?     5226           
  Branches          ?        0           
=========================================
  Hits              ?     2903           
  Misses            ?     2323           
  Partials          ?        0
Impacted Files Coverage Δ
src/assets/JSAsset.js 69.81% <ø> (ø)
src/packagers/JSConcatPackager.js 4.87% <0%> (ø)
src/scope-hoisting/concat.js 8.33% <100%> (ø)
src/visitors/fs.js 8.33% <100%> (ø)
src/visitors/env.js 100% <100%> (ø)
src/packagers/JSPackager.js 97.29% <100%> (ø)
src/visitors/dependencies.js 94.57% <100%> (ø)
src/scope-hoisting/hoist.js 7.52% <16.66%> (ø)
src/transforms/babel.js 95.4% <95.65%> (ø)
src/transforms/babelASTConverter.js 97.56% <97.56%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0df562...2173572. Read the comment docs.

package.json Outdated
"babel-preset-env": "^1.7.0",
"babel-template": "^6.26.0",
"babel-traverse": "^6.26.0",
"babel-types": "^6.26.0",
"babylon": "^6.17.4",
Copy link
Member

Choose a reason for hiding this comment

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

remove this too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot that one, thanks :)

@@ -203,21 +245,31 @@ async function getBabelRc(asset, isSource) {

// If specified as an array, override the config with the one specified
if (Array.isArray(babelify) && babelify[1]) {
return babelify[1];
return {
version: 6,
Copy link
Member

Choose a reason for hiding this comment

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

I guess we should test this the same way as above - it's possible that there could be babel 7 plugins in here too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, just thought legacy, people won't use that for babel 7. But sure I'll make it check the plugins

}

function getPluginName(p) {
return Array.isArray(p) ? p[0] : p;
}

function testPluginArrayForBabelScope(plugins) {
for (let plugin of plugins) {
if (getPluginName(plugin).startsWith('@babel/')) {
Copy link
Member

Choose a reason for hiding this comment

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

Does babel 7 require the scope or will they automatically add it if you just write env? If so, then this check won't work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Babel docs:

You can still use the shorthand way of specifying a preset or plugin. However because of the switch to scoped packages, you still have to specify the @babel/ just like if you had your own preset to add to the config.

So yes it's required, although it's not 100% reliable as third party libraries will still have babel-....

Copy link
Member

Choose a reason for hiding this comment

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

ok

"babel-template": "^6.26.0",
"babel-traverse": "^6.26.0",
"babel-types": "^6.26.0",
"babylon": "^6.17.4",
"babylon-walk": "^1.0.2",
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we remove this, or do a PR to their branch to update to @babel/types instead of babel-types

It might cause issues in certain cases, as babel-types has changed slightly

Copy link
Member

Choose a reason for hiding this comment

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

worth doing some perf testing. babylon-walk was WAY faster than babel-traverse for some cases where we don't need scope tracking and things.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm pretty sure it will still be slower, haven't tested it. I did however open up a PR to babylon-walk pugjs/babel-walk#2

@DeMoorJasper
Copy link
Member Author

DeMoorJasper commented Aug 31, 2018

I'm probably not gonna be adding any more features to this PR, as it's becoming quite big. (Might open up a new one to figure out the config things)

Feel free to merge whenever. (or request changes)

return babelCore.parse(code, options);

if (options.babelVersion === 6) {
let babylon = await localRequire('babylon', this.name);
Copy link
Member

Choose a reason for hiding this comment

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

Why do we still need babylon? I think we should always parse with babel 7 and convert the AST as needed. Our internal plugins will all be using babel 7, and some of them run before custom babel transforms.

Copy link
Member Author

Choose a reason for hiding this comment

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

I left it there for plugins that run on ast creation. Not sure if that's really necessary


asset.ast = babelASTConverter(asset.ast, 6);

return babelTransform(asset, 7);
Copy link
Member

Choose a reason for hiding this comment

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

Won't this result in transforming every file twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but it will only run babel 6 transforms on the first run, convert to babel 7 and run the internal config. Thought this was the expected behaviour.
I might have made a mistake though, not entirely sure this is the proper way to do this.

Copy link
Member

Choose a reason for hiding this comment

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

oh I see. yeah that's how it should work.

@devongovett devongovett merged commit e89f06c into babel7 Sep 4, 2018
@devongovett devongovett deleted the jasper/remove-babel6 branch September 4, 2018 01:19
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.

None yet

3 participants