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

Upgrade to Babel 6 #333

Closed
wants to merge 2 commits into from
Closed

Upgrade to Babel 6 #333

wants to merge 2 commits into from

Conversation

jamestalmage
Copy link
Contributor

#221 Was so outdated it was easier just to start fresh than rebase.

There is one remaining issue from the comments on that PR. This PR still does not intelligently drop babel-plugin-transform-async-to-generator. That plugin is now buried in the stage-3 preset, and I'm not sure if there is a way to still use that preset, but tell it to exclude that particular plugin. Recreating that preset would be fairly trivial (it aggregates only two plugins). I am not sure that is a good plan though. By definition, the stage-3 preset is subject to change over time, and it seems to me that we want to automatically stay up to date. We already track this with #148. Is there something special about async-to-generator that we feel we must handle it specially now before we "do it right" with #148?

@jamestalmage jamestalmage mentioned this pull request Dec 15, 2015
"babel-plugin-transform-runtime": "^6.3.13",
"babel-preset-es2015": "^6.3.13",
"babel-preset-stage-2": "^6.3.13",
"babel-preset-stage-3": "^6.3.13",
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-stage-2 includes babel-preset-stage-3 so you only need stage-2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.
When I started, I thought I only needed stage-3. I was unaware Object Rest/Spread arguments were stage-2.

@sindresorhus
Copy link
Member

Is there something special about async-to-generator that we feel we must handle it specially now before we "do it right"

I don't know how that plugin works now, but we need to polyfill generator support on Node.js 0.10. I don't really have a strong opinion of how that's solved though.

@jamestalmage
Copy link
Contributor Author

I read it wrong. We were dynamically blacklisting regenerator function, not async-to-generator. The decision to do that dynamically was made back in #61. Now that we reformat stack traces with source-map-support I don't think it is that big of deal.

@sindresorhus
Copy link
Member

@jamestalmage We only want to include regenerator on Node.js 0.10 though, as it's a lot slower than native generators.

@jamestalmage
Copy link
Contributor Author

OK,
The Babel docs no longer mention the ability to black list particular plugins. Unless it still exists and is just currently undocumented, our only option will be to build an equivalent to the es2015 preset ourselves. Not exceptionally difficult, but it will add to our overhead as the preset changes over time.

it's a lot slower than native generators.

Do we have good data on that? (I believe it, just curious).

@jamestalmage
Copy link
Contributor Author

Hmm - I'm seeing a blacklist option here, wonder if that is babelify specific or not. Need to check it out.

@@ -3,5 +3,7 @@ node_js:
- 'stable'
- '0.12'
- '0.10'
before_install:
- 'npm i -g npm@3'
Copy link
Member

Choose a reason for hiding this comment

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

- 'npm i -g npm@latest'

@sindresorhus
Copy link
Member

There's something mentioning blacklist here: https://github.com/babel/babel/blob/59f5e7f218ffa788e6e3f320a3e7e0cbe6d50894/packages/babel-plugin-transform-runtime/src/index.js#L14-L22

Or maybe we could just do a PR to babel-plugin-transform-runtime to only add the regenerator include when needed (based on has-generator).

I would prefer we didn't have to maintain our own preset.

@jamestalmage
Copy link
Contributor Author

transform-runtime would not be the right place. That only transforms what would otherwise be polyfilled globals into require statements to avoid polluting the global namespace:

var x = new Promise(...)
// => 
var runtime = require('runtime');
var x = new runtime.Promise(...);

I think it would need to be an config param to babel-preset-es2015 (can you even pass config params to presets?), or to whichever module would be the appropriate target for a blacklist option. Probably something near or around the OptionManager class; it looks like it does some work loading/configuring plugins.

I would prefer we didn't have to maintain our own preset.

Yeah - that was my feeling.

@sindresorhus
Copy link
Member

I think it would need to be an config param to babel-preset-es2015 (can you even pass config params to presets?)

I read somewhere on the Babel issue tracker they would be open to that.

@sindresorhus
Copy link
Member

A lot of stuff are blocked by Babel 6 support. I'm trying to think what's the minimum we can do to land this? Maybe we can just have regenerator even for newer Node.js version and open a ticket about submitting an option to babel-preset-es2015?

@sindresorhus sindresorhus changed the title (yet another) Switch to Babel 6 Switch to Babel 6 Dec 21, 2015
@jamestalmage
Copy link
Contributor Author

Maybe we can just have regenerator even for newer Node.js version and open a ticket about submitting an option to babel-preset-es2015?

👍

@sindresorhus
Copy link
Member

Done: #348

@jamestalmage Anything else before we can merge this?

@sindresorhus sindresorhus changed the title Switch to Babel 6 Upgrade to Babel 6 Dec 21, 2015
@fernandofleury
Copy link

Any feedback when this feature might be finished? Any points we could help?

@jamestalmage
Copy link
Contributor Author

Anything else before we can merge this?

That depends on our priorities. It breaks Babel 5 support (You can't use babel-5 to instrument your production code). I have no good solution for that.

Perhaps we could simply detect which Babel version resolves for a given test file, and then conditionally provide a Babel6 or Babel5 config based on what we find.

Or we just rip off the band-aid and go for Babel 6.

@sindresorhus
Copy link
Member

Or we just rip off the band-aid and go for Babel 6.

👍 I'm not too concerned with that as Babel 5 is not supported anymore and people will have to upgrade sooner than later and they can just stay on AVA 0.8.0 in the meantime.

@sindresorhus
Copy link
Member

Landed. Yay! :)

jamestalmage added a commit that referenced this pull request Dec 22, 2015
sindresorhus added a commit that referenced this pull request Dec 22, 2015
@jamestalmage jamestalmage deleted the babel-6 branch January 19, 2016 00:33
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.

4 participants