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

Only reference helpers from external/runtime helpers if they are known to be available. #8398

Merged
merged 4 commits into from Aug 3, 2018

Conversation

@loganfsmyth
Copy link
Member

loganfsmyth commented Jul 29, 2018

Q                       A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change? Y
Minor: New Feature? Y
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

Why

This PR aims to resolve a specific problem. Babel's helpers live as part of @babel/core conceptually, or indirectly from @babel/helpers, but when users use @babel/plugin-transform-runtime or @babel/plugin-external-helpers, the helpers actually load from a static file like @babe/runtime/helpers/classCallCheck.

While very handy, this has the downside that we don't actually know what version of a helper we're going to get, and even more frustratingly, we don't actually know if the helper even exists in the runtime, for instance if we've recently added new helpers. In 6.x, this meant that we could add a new transform for instance, along with a helper for it, but users using @babel/runtime would get the extremely unhelpful error that their import didn't find the file.

It also meant that there was literally no way for a plugin to check if a helper was available. If transform-runtime or similar plugin was enabled, any attempt to load a helper would insert a reference, even if it was totally broken, or you'd just typoed the name of a helper or something.

This PR aims to resolve that problem by changing the expectations around intercepting runtime helper loading. If an override function is given, it is that function's responsibility to only insert a reference to a helper if it knows the helper exists. That means that plugins themselves can more easily evolve over time. For instance, a plugin can attempt to add a helper, and if it doesn't exist, it could fall back to inserting a different helper instead, or it could provide useful feedback to users saying that they'll need to update their version of @babel/core in order to use this new helper.

How

This PR accomplishes it's goal by having @babel/core itself be a source of truth for what helpers exist, and what helpers became available when. When a user does

plugins: [
  ['@babel/transform-runtime']
]

they are declaring "load all helpers from the runtime, BUT only if they were available at the release of whichever version this PR lands in.

What this means is that, in the future if we make a new plugin with some new helper, it will not be loaded via the runtime at all. It would be up to the user to explicitly do

plugins: [
  ['@babel/transform-runtime', { version: "7.4.2"}] // or a range
]

to essentially say "I promise that I have version 7.4.2 of @babel/runtime available, so any helpers that existed in 7.4.2 are free to be loaded from this runtime.

Downsides

The main downside of this approach is that it's more work for users to have to manually provide the version. In a perfect world, I'd love for @babel/transform-runtime to actually try to look at the user's package.json or something, and I thought about doing that for this PR, but I ended up deciding that it complicated things too much for now. We can always explore adding it as an opt-in feature during 7.x's lifetime.

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Jul 29, 2018

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

export default helpers;

helpers.typeof = () => template.program.ast`
"minVersion: 7.0.0-beta.0";

This comment has been minimized.

Copy link
@Andarist

Andarist Jul 30, 2018

Member

imho it's slightly weird to use directive-like for this, making it a property of the helper (or wrapping helpers in objects with this and smth like create method) would make it less magical

This comment has been minimized.

Copy link
@xtuc

xtuc Jul 31, 2018

Member

I agree, what about helpers.typeof.minVersion = "7.0.0-beta.0"?

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Aug 1, 2018

Author Member

I was being lazy :P Since none of this is actually part of the public API, I figured refactoring it later would be trivial. I'll see if I can make it better now though.

Copy link
Member

nicolo-ribaudo left a comment

Will this PR allow us to introduce breaking change to the helpers, by incrementing the minVersion?

throw new Error(
"Babel 7.0.0-beta.56 has dropped support for the 'helpersNamespace' utility." +
"If you are using @babel/plugin-external-helpers you will need to use a newer " +
"version than the one you currently have installed.",

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jul 30, 2018

Member

This error message should suggest plugin authors to use helperGenerator instead.

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Aug 1, 2018

Author Member

Good call.

return (
typeof versionRange !== "string" ||
(!semver.intersects(`<${minVersion}`, versionRange) &&
!semver.intersects(`>=8.0.0`, versionRange))

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jul 30, 2018

Member

Why is this second check needed? Some helpers almost never change (e.g. the typeof one), so I would expect it to be possible that some helpers are compatible even between major versions.

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Aug 1, 2018

Author Member

My concern is that while that is indeed likely, it's not a given. Changing a helper in a breaking way in a new major version is still entirely possible, so my limiting this range clearly, we can be sure that if someone has @babel/core@8 installed, and they try to use @babel/runtime@7.3 or whatever, we'll just inline the helpers instead of inserting the runtime imports.

}

return (
typeof versionRange !== "string" ||

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Jul 30, 2018

Member

Maybe this should also work with integers, like the api.assertVersion function does?

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Aug 1, 2018

Author Member

We could, though I'm not sure it's worth it. This is an API I don't think many will use, and always expecting semver strings does simplify things a bit.

@@ -14,6 +14,7 @@ export default declare((api, options) => {
regenerator,
useBuiltIns,
useESModules,
version: runtimeVersion = "7.0.0-beta.0",

This comment has been minimized.

Copy link
@xtuc

xtuc Jul 31, 2018

Member

In availableHelper is versionRange is optional, could we omit the default argument here?

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Aug 1, 2018

Author Member

In availableHelper, if you omit the version, it just checks if the helper exists in the current version of core, rather than if it was historically available at a particular version. If we left this blank, we'd essentially always insert the helper imports, even when it was invalid.

export default helpers;

helpers.typeof = () => template.program.ast`
"minVersion: 7.0.0-beta.0";

This comment has been minimized.

Copy link
@xtuc

xtuc Jul 31, 2018

Member

I agree, what about helpers.typeof.minVersion = "7.0.0-beta.0"?

@@ -247,6 +252,15 @@ function loadHelper(name) {
globals: metadata.globals,
};
},
minVersion() {
return fn()

This comment has been minimized.

Copy link
@xtuc

xtuc Jul 31, 2018

Member

And following my previous comment. Here: fn.minVersion.

@hzoo hzoo added this to the Babel 7 RC milestone Aug 1, 2018
@loganfsmyth loganfsmyth force-pushed the loganfsmyth:helper-whitelisting branch from 156c1dd to 4042fcd Aug 2, 2018
@loganfsmyth

This comment has been minimized.

Copy link
Member Author

loganfsmyth commented Aug 2, 2018

@nicolo-ribaudo

Will this PR allow us to introduce breaking change to the helpers, by incrementing the minVersion?

No, that's not my intention with this PR, but what it will allow us to do is add a new helper with a new API, and use that new helper as a replacement for the old one in a safe way.

@loganfsmyth loganfsmyth force-pushed the loganfsmyth:helper-whitelisting branch from 4042fcd to 5cfd383 Aug 3, 2018
@loganfsmyth loganfsmyth closed this Aug 3, 2018
@loganfsmyth loganfsmyth force-pushed the loganfsmyth:helper-whitelisting branch from 5cfd383 to adca165 Aug 3, 2018
@loganfsmyth loganfsmyth reopened this Aug 3, 2018
@loganfsmyth loganfsmyth force-pushed the loganfsmyth:helper-whitelisting branch from 6de201e to e2d64f1 Aug 3, 2018
@loganfsmyth loganfsmyth merged commit 8173b6e into babel:master Aug 3, 2018
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 80.55% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@loganfsmyth loganfsmyth deleted the loganfsmyth:helper-whitelisting branch Aug 3, 2018
@DreRandaci

This comment has been minimized.

Copy link

DreRandaci commented Aug 31, 2018

@loganfsmyth

I am implementing babel-plugin-external-helpers into a Rollup build process and am getting the Babel 7.0.0-beta.56 has dropped support for the 'helpersNamespace' utility error when I do a build. I'm using the rollup-plugin-babel library and am new to implementing Rollup in general, so I don't doubt I'm doing something wrong. Do I need to specify an external-helpers version in my .babelrc plugins? I am following the Rollup docs for this.

.babelrc file

{
  "presets": [
    [
      "env",
      {
        "modules": false
      }
    ]
  ],
  "plugins": [
    "external-helpers"
  ]
}

package.json

"devDependencies": {
    "@babel/core": "^7.0.0",
    "babel-plugin-external-helpers": "^6.22.0",
    "babel-preset-env": "^1.7.0",
    "eslint": "^5.4.0",
    "eslint-config-prettier": "^3.0.1",
    "eslint-config-standard": "^12.0.0",
    "eslint-plugin-import": "^2.14.0",
    "eslint-plugin-node": "^7.0.1",
    "eslint-plugin-prettier": "^2.6.2",
    "eslint-plugin-promise": "^4.0.0",
    "eslint-plugin-standard": "^4.0.0",
    "prettier": "1.14.2",
    "rollup": "^0.65.0",
    "rollup-plugin-babel": "^4.0.2",
    "rollup-plugin-size-snapshot": "^0.6.1"
  }
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Aug 31, 2018

You can use @babel/transform-runtime and @babel/runtime: the former will inject import _classCallCheck from "@babel/runtime/helpers/classCallCheck for every needed helper, and then Rollup will bundle them with your code.

@DreRandaci

This comment has been minimized.

Copy link

DreRandaci commented Aug 31, 2018

@nico29 I changed my .babelrc config to use the @babel/preset-env preset and got rid of the "external-helpers" plugin, which seems to have resolved the issue (since I'm using babel 7). Does your solution target babel < 6? Or is it for deduping the inserted helpers?

EDIT: I also updated to rollup-plugin-babel@next

Updated .babelrc

{
  "presets": [
    [
      "@babel/preset-env",
      {
        "modules": false
      }
    ]
  ],
  "plugins": []
}
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Aug 31, 2018

Babel by default injects the helpers in every file. @babel/plugin-transform-runtime avoids it by allowing the helpers to be imported, so that they can be deduped. babel-plugin-external-helpers had the same goal, but it relied on a global variable.

@Andarist

This comment has been minimized.

Copy link
Member

Andarist commented Aug 31, 2018

Just to add what kinda got already said - @babel/plugin-external-helpers is no longer needed to dedupe helpers when using babel@7 and corresponding rollup-plugin-babel@4.

Need to update rollup docs (had no idea they have section about this) to explain difference between configuring for babel@6 and babel@7.

@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
7 participants
You can’t perform that action at this time.