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

Split @babel/runtime into 2 modules via @babel/runtime-corejs2 #8266

Merged
merged 6 commits into from Aug 3, 2018

Conversation

@loganfsmyth
Copy link
Member

@loganfsmyth loganfsmyth commented Jul 5, 2018

Q                       A
Fixed Issues? Fixes #8155
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR EDIT by @hzoo: babel/website#1714
Any Dependency Changes?
License MIT

In Babel 6, we had @babel/runtime which always uses core-js in its helpers, meaning that anyone who wants to use our helpers in a centralized modular way had to also use core-js, which isn't always ideal.

Early in the Babel 7 alpha, we added a useBuiltins: true option that would use separate copies of the helpers that would skip loading core-js, but this still requires users to have an indirect npm dependency on core-js.

This PR splits our setup into two separate runtimes:

  • 'plugins': [
      ['transform-runtime'],
    ]
    
    with npm install --save @babel/runtime that just includes our helpers, and relies on users to globally polyfill any APIs they need.
  • 'plugins': [
      ['transform-runtime', { corejs: 2 }],
    ]
    
    with npm install --save @babel/runtime-corejs2 that includes everything @babel/runtime does, but also includes a passthrough API for corejs@2.x and rewrites all of the helper modules to reference core-js.
@babel-bot
Copy link
Collaborator

@babel-bot babel-bot commented Jul 5, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/8756/

@babel babel deleted a comment from babel-bot Jul 5, 2018
@loganfsmyth loganfsmyth force-pushed the loganfsmyth:runtime-refactor branch from 5fd6aec to b95564b Jul 5, 2018
for (const dep of helpers.getDependencies(helperName)) {
const id = (dependencies[dep] = t.identifier(t.toIdentifier(dep)));
tree.body.push(template.statement.ast`
var ${id} = require("${`./${dep}`}");

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jul 5, 2018
Member

Some helpers redefine the exported function. Does this work in that case?

This comment has been minimized.

@loganfsmyth

loganfsmyth Jul 5, 2018
Author Member

Yeah, it's a bug, but it's existing and tracked in #8087

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jul 5, 2018
Member

Oh ok, I didn't realize you just moved around this code

regenerator,
useBuiltIns,
useESModules,
corejsVersion = false,

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jul 5, 2018
Member

Maybe this option should be named just corejs? corejs: 2 looks as good as corejsVersion: 2 to me, but corejs: false looks better than corejsVersion: false.

This comment has been minimized.

@loganfsmyth

loganfsmyth Jul 5, 2018
Author Member

Sounds good. I was on the fence.

@zloirock
Copy link
Member

@zloirock zloirock commented Jul 5, 2018

  • Please, use kebab-case for core-js where it's possible for consistency with package / repo name and simplify of searching.
  • I haven't any plans about long time support of core-js@2 after core-js@3 release and creating runtime package for babel@7 which use core-js@2 could cause problems in the future.
@loganfsmyth
Copy link
Member Author

@loganfsmyth loganfsmyth commented Jul 5, 2018

@zloirock

use kebab-case for core-js where it's possible

Do you have strong feelings about specific cases in this PR? The primary issue is that I think the extra dash makes things harder in some cases here, like babel-runtime-core-js-2 is definitely harder to read than -corejs2. Same if we rename the corejsVersion option to corejs, using the dash would be frustrating since users would have to quote the option name.

I haven't any plans about long time support of core-js@2 after core-js@3 release

I think that's fine. I thought about making it just babel-runtime-corejs, but my concern was that people may want control over it because they may have other packages built with Babel 6's babel-runtime, and if we don't provide a runtime that uses core-js@2 then they'll be forced to use two parallel versions of core-js at the same time, which would increase their bundle sizes a lot. I'd still like to get core-js@3 landed and recommend it over the 2.x version.

.gitignore Outdated
!/packages/babel-runtime-corejs2/helpers/es6/iterableToArray.js
!/packages/babel-runtime-corejs2/helpers/es6/temporalRef.js
/packages/babel-runtime-corejs2/core-js/**/*.js
!/packages/babel-runtime-corejs2/core-js/map.js

This comment has been minimized.

@hzoo

hzoo Jul 5, 2018
Member

For anyone else reviewing, the reason why I added these ! gitignore's earlier is so that we are able to view the changes in output for the runtime since we would change the build script and it had a bug before and we never viewed the diff. But maybe there is a better way to do this since it's hardcoded. Basically we want to catch issues with the script

@hzoo
hzoo approved these changes Jul 5, 2018
Copy link
Member

@hzoo hzoo left a comment

looks good, just will want to document these changes on the website docs for all these packages, and then add to the migration guide which I can help with as well.

@@ -0,0 +1,14 @@
var _Map = require("@babel/runtime-corejs2/core-js/map");

This comment has been minimized.

@hzoo

hzoo Jul 5, 2018
Member

This is just a nit and maybe not worth looking into, but do we want to just make the path require("@babel/runtime-corejs2/map")?

This comment has been minimized.

@loganfsmyth

loganfsmyth Jul 7, 2018
Author Member

Hmm, I don't love having core-js repeated twice, but I do think having the core-js stuff grouped into a folder is nice. Do you just want to make it shorter, or is the repetition the part you want to avoid? Thoughts on other names?

@hzoo hzoo mentioned this pull request Jul 5, 2018
const helperFilename = path.join(
pkgDirname,
"helpers",
esm ? "es6" : "",

This comment has been minimized.

@Andarist

Andarist Jul 5, 2018
Member

if the option is named esm then this directory should be IMHO named esm too, es6 makes it more confusing because othen than module format those files wont be written using es6 syntax

This comment has been minimized.

@loganfsmyth

loganfsmyth Jul 7, 2018
Author Member

Sounds good to me. This was just copied over as it was from babel/runtime

@@ -0,0 +1,158 @@
"use strict";

This comment has been minimized.

@Andarist

Andarist Jul 5, 2018
Member

imho it's really confusing that this build script is inside of babel-plugin-transform-runtime package

I understand that this script creates those files in more than 1 package, but as such I would see it being in a top-level scripts and not here

This comment has been minimized.

@loganfsmyth

loganfsmyth Jul 7, 2018
Author Member

The transform and the build script share the same source of data, which was my motivation. If we split it out, the build script will still have to reach into the transform to get that data, and doing that felt equally awkward.

This comment has been minimized.

@Andarist

Andarist Jul 7, 2018
Member

In that case - feel free to go with whatever u prefer, I would still prefer to have this top-level, but I'm not super attached to it now (because of the reasons u gave)

This comment has been minimized.

@hzoo

hzoo Jul 9, 2018
Member

I guess we should explain somewhere about running this

@Andarist
Copy link
Member

@Andarist Andarist commented Jul 5, 2018

Could u explain how /es6/* helpers are supposed to be used in comparison to .mjs ones? I also can't see .mjs being created with the script in this PR - when they are created?

@hzoo hzoo added the Priority: High label Jul 7, 2018
@loganfsmyth
Copy link
Member Author

@loganfsmyth loganfsmyth commented Jul 7, 2018

@Andarist This is just moving the existing logic. We don't currently offer .mjs helpers. I considered adding them, but was concerned about it potentially being more painful for users. It's also not too bad to add .mjs versions later, I think.

@Andarist
Copy link
Member

@Andarist Andarist commented Jul 7, 2018

This gave me the conclusion that .mjs is somehow provided - I guess this is some kind of smoke test checked into the repo?

We don't currently offer .mjs helpers. I considered adding them, but was concerned about it potentially being more painful for users

Why?

It's also not too bad to add .mjs versions later, I think.

Agreed.

@loganfsmyth
Copy link
Member Author

@loganfsmyth loganfsmyth commented Jul 7, 2018

This gave me the conclusion

Oh shoot, that's left over from when I was experimenting with support for it. It shouldn't have been committed.

Why?

Mostly that the semantics of .mjs files still aren't official anywhere, especially around how they are loaded, and which one is loaded when, for instance if you don't provide a file extension.

@xtuc
xtuc approved these changes Jul 9, 2018
Copy link
Member

@xtuc xtuc left a comment

looks good!

@loganfsmyth loganfsmyth force-pushed the loganfsmyth:runtime-refactor branch 2 times, most recently from 0e70926 to cc8b65a Jul 15, 2018
@loganfsmyth
Copy link
Member Author

@loganfsmyth loganfsmyth commented Jul 15, 2018

Alright I have:

  • Renamed the es6 folder to esm
  • Renamed the corejsVersion option to corejs
  • Removed accidentally-included .mjs files
  • Deleted babel-runtime/core-js.js file (and not moved it to babel-runtime-corejs2 since it isn't used as far as I can see.

@hzoo You had comments about the core-js/ folder making core-js be repeated twice, is that still a concern you have?

@zloirock Any thoughts on my responses to your thoughts above?

@hzoo
Copy link
Member

@hzoo hzoo commented Jul 15, 2018

LGTM

core-js/ folder making core-js be repeated twice, is that still a concern you have?

I'm ok if we don't change it, I understand it's nice to have it sorted by folder. Not sure I have a better suggestion

@zloirock
Copy link
Member

@zloirock zloirock commented Jul 15, 2018

@loganfsmyth sorry for delayed reply. My opinion about kebab-case for core-js enough strong by the reason of searching. I don't think that it could make reading more difficult. I could change my opinion if a serious part of maintainers against kebab-case here.

@loganfsmyth
Copy link
Member Author

@loganfsmyth loganfsmyth commented Jul 15, 2018

@zloirock Are there specific instances that you see in this PR that you disagree with? It would make discussions easier to have specific cases to discuss.

@benjamn benjamn mentioned this pull request Aug 6, 2018
4 of 4 tasks complete
@insin insin mentioned this pull request Aug 7, 2018
7 of 8 tasks complete
@insin
Copy link

@insin insin commented Aug 7, 2018

I don't think anybody really uses the current moduleName option now

nwb depended on this to have the transform-runtime plugin generate imports which pointed to the absolute path to babel-runtime in nwb's own dependencies, allowing it to serve and build stuff with all devDependencies managed in a global install - is there an alternative way to get Babel to resolve the runtime module when it's not going to be available in require() scope?

@loganfsmyth loganfsmyth deleted the loganfsmyth:runtime-refactor branch Aug 7, 2018
@loganfsmyth
Copy link
Member Author

@loganfsmyth loganfsmyth commented Aug 7, 2018

@insin The original intention of moduleName was to allow custom runtimes, which isn't something we're looking to support in 7.x, but the usecase of passing an absolute path for the runtime itself seems entirely reasonable.

I'd be happy to have an option of some kind (for transform-runtime) to have Babel inject absolute paths. How would you feel about a useAbsoluteRuntime option? My preference would probably be to allow both:

  • useAbsoluteRuntime: true
    • If the plugin is passed in the programmatic args, it resolves relative to Babel's cwd
    • If the plugin is passed via a config file, it resolve relative to that file
  • useAbsoluteRuntime: "/some/abs/dir"
    • Resolve the runtime relative to some specific directory, which for nwb could just be __dirname

Thoughts?

@insin
Copy link

@insin insin commented Aug 7, 2018

@loganfsmyth Those both look good to me 👍

Would that also support passing the absolute path, as in path.dirname(require.resolve('@babel/runtime/package'))?

@trusktr
Copy link

@trusktr trusktr commented Aug 7, 2018

Hello awesome Babel devs, it'd be awesome to avoid in-range breaking changes, and it would be highly valued and appreciated (so it doesn't break all the stuffs and the things)!

Luckily, I share a single build config with my projects, so fixing them was easy. 😃

Is there a way that during an npm install we can get a very noticeable and outstanding message about the breaking change? Perhaps an install hook can detect the version from package.json, and putput the message if the in-range version is lower than the one that introduces the change.

That would be very useful. If that is already the case, then I missed it and it wasn't noticeable enough.

trusktr added a commit to trusktr/lowclass that referenced this pull request Aug 7, 2018
@benjamn
Copy link
Contributor

@benjamn benjamn commented Aug 8, 2018

As disruptive as these changes might be, I think they're totally fair game for a beta release. Being more careful about breaking changes during beta just means it takes longer for Babel 7 to be officially released, which is the goal we're all aiming for. Once we drop the -beta.n suffix, we can go back to normal semantic versioning, which probably would have saved you from this particular pain.

@loganfsmyth
Copy link
Member Author

@loganfsmyth loganfsmyth commented Aug 8, 2018

@trusktr

it'd be awesome to avoid in-range breaking changes

While I totally understand that it's frustrating, the entire reason for being in -beta is because that specifically exists to signal that something may have breaking changes.

For instance https://semver.org/ calls out

A pre-release version indicates that the version is unstable and might not satisfy the intended compatibility requirements as denoted by its associated normal version.

If it were only ever possible to make breaking changes in exact major version bumps, prerelease versions wouldn't exist in the first place.

Similarly, https://www.npmjs.com/package/semver#prerelease-tags

The purpose for this behavior is twofold. First, prerelease versions frequently are updated very quickly, and contain many breaking changes that are (by the author's design) not yet fit for public consumption. Therefore, by default, they are excluded from range matching semantics.

Second, a user who has opted into using a prerelease version has clearly indicated the intent to use that specific set of alpha/beta/rc versions. By including a prerelease tag in the range, the user is indicating that they are aware of the risk.

As @hzoo mentioned above, npm's behavior around prereleases isn't quite ideal right now, and we've filed npm/rfcs#14 to try to have that addressed in future versions.

Once we drop the -beta and release a proper 7.0.0 we'll absolutely be avoiding any known regressions in behavior.

@loganfsmyth
Copy link
Member Author

@loganfsmyth loganfsmyth commented Aug 8, 2018

@insin

Would that also support passing the absolute path, as in path.dirname(require.resolve('@babel/runtime/package'))?

It would not, and I don't know that it's something I'd want to allow, because it should be the runtime transform itself deciding the actual name of the runtime, at least in my opinion. In the context of nwb though, since require.resolve resolves a package relative to __dirname, you'd get exactly the behavior of your current implementation by doing

plugins: [
  ['@babel/plugin-transform-runtime', { absoluteRuntime: __dirname }],
]

to resolve the runtime relative to __dirname.

@loganfsmyth
Copy link
Member Author

@loganfsmyth loganfsmyth commented Aug 8, 2018

For the absolute-path discussion, I created #8435, so let's continue that over there.

@trusktr
Copy link

@trusktr trusktr commented Aug 8, 2018

-beta is because that specifically exists to signal that something may have breaking changes.

You're right. This is an interesting case: v7 has been in Beta so long that people are using it like it's not Beta.

@hijiangtao
Copy link

@hijiangtao hijiangtao commented Dec 10, 2018

Alright I have:

  • Renamed the es6 folder to esm
  • Renamed the corejsVersion option to corejs
  • Removed accidentally-included .mjs files
  • Deleted babel-runtime/core-js.js file (and not moved it to babel-runtime-corejs2 since it isn't used as far as I can see.

@hzoo You had comments about the core-js/ folder making core-js be repeated twice, is that still a concern you have?

@zloirock Any thoughts on my responses to your thoughts above?

Is there any compatible writting for Renamed the es6 folder to esm?

I found some packages would throw an error like:

...

Module not found: 
Can't resolve '@babel/runtime/helpers/esm/assertThisInitialized' in 
'/site/node_modules/_luma.gl@6.3.1@luma.gl/dist/esm/webgl'

...

in which luma.gl@6.3.1 requires a different version of @babel/runtime with my project (the project itself requires an old version of @babel/runtime before v7.0.0-beta56).

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Dec 10, 2018

If you (or a dependency who uses directly uses Babel) require a beta version, all your babel packages and the packages used by your deps need to match that beta version.
I strongly suggest you to update your dependencies.

@hijiangtao
Copy link

@hijiangtao hijiangtao commented Dec 10, 2018

If you (or a dependency who uses directly uses Babel) require a beta version, all your babel packages and the packages used by your deps need to match that beta version.
I strongly suggest you to update your dependencies.

Thanks for your reminding, I tried upgraded @babel/runtime's version in my repo, however the same problem reproduced in some dependencies inside the dependencies of my repo, which has got out of my control (I guess those dependencies require an old version of @babel/runtime and use the grammar like .../es6/..., a little award to express this situation, kind of recall hell in some aspect).

I'll look and see if there's some method to solve it, thanks.

danez added a commit to babel/babel-upgrade that referenced this pull request Mar 23, 2019
* Split @babel/runtime into 2 modules via @babel/runtime-corejs2

babel/babel#8266

* Only add the `corejs` option when needed, display a warning if not sure

* Tpo

* Update upgradeConfig.js

* Fix merge
@lock lock bot added the outdated label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

You can’t perform that action at this time.