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

fix: Don't transpile ES2018 symbol properties #9650

Merged
merged 2 commits into from Mar 10, 2019

Conversation

taion
Copy link
Contributor

@taion taion commented Mar 7, 2019

Q                       A
Fixed Issues? #4783 (again)
Patch: Bug Fix? 👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? No, per #5195
Documentation PR Link
Any Dependency Changes?
License MIT

This is a repeat of #5195 to work around the same upstream issue.

@babel-bot
Copy link
Collaborator

babel-bot commented Mar 7, 2019

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

@ljharb ljharb changed the title fix: Don't transpile ES7 symbol properties fix: Don't transpile ES2018 symbol properties Mar 7, 2019
Copy link
Member

@danez danez left a comment

Choose a reason for hiding this comment

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

Seems once we update to v3 this can be undone.

@zloirock
Copy link
Member

zloirock commented Mar 7, 2019

It could be a breaking change. If someone uses old transform and new runtime, entry point @babel/runtime/core-js/symbol/async-iterator will be missed.

@taion
Copy link
Contributor Author

taion commented Mar 7, 2019

@zloirock It won't be – the runtime transform will see that Symbol gets pulled in, and it will import the full symbol polyfill, which will work as before. This was the behavior in Babel v6 with #5195.

@taion
Copy link
Contributor Author

taion commented Mar 7, 2019

And, yeah, ideally we'll kill this hack for good once we upgrade to core-js v3.

@nicolo-ribaudo
Copy link
Member

We should hardcode it in build-dist, so that it isn't removed from the package.

@taion
Copy link
Contributor Author

taion commented Mar 7, 2019

@nicolo-ribaudo Hardcode what?

@taion
Copy link
Contributor Author

taion commented Mar 7, 2019

The reason this isn't a breaking change is because we get the "full" Symbol polyfill rather than just the one for Symbol.asyncIterator.

In the below, note that it's now _Symbol.asyncIterator instead of _Symbol$asyncIterator. But this is fine – that's properly defined and present.

Before

var _Symbol$iterator = require("../core-js/symbol/iterator");

var _Symbol$asyncIterator = require("../core-js/symbol/async-iterator");

var _Symbol = require("../core-js/symbol");

function _asyncIterator(iterable) {
  var method;

  if (typeof _Symbol === "function") {
    if (_Symbol$asyncIterator) {
      method = iterable[_Symbol$asyncIterator];
      if (method != null) return method.call(iterable);
    }

    if (_Symbol$iterator) {
      method = iterable[_Symbol$iterator];
      if (method != null) return method.call(iterable);
    }
  }

  throw new TypeError("Object is not async iterable");
}

module.exports = _asyncIterator;

After

var _Symbol$iterator = require("../core-js/symbol/iterator");

var _Symbol = require("../core-js/symbol");

function _asyncIterator(iterable) {
  var method;

  if (typeof _Symbol === "function") {
    if (_Symbol.asyncIterator) {
      method = iterable[_Symbol.asyncIterator];
      if (method != null) return method.call(iterable);
    }

    if (_Symbol$iterator) {
      method = iterable[_Symbol$iterator];
      if (method != null) return method.call(iterable);
    }
  }

  throw new TypeError("Object is not async iterable");
}

module.exports = _asyncIterator;

@zloirock
Copy link
Member

zloirock commented Mar 7, 2019

I mean that someone could have @babel/transform-runtime pinned to, for example, 7.3 (code sample Before) and @babel/runtime@7.4 without @babel/runtime/core-js/symbol/async-iterator entry point. And use Symbol.asyncIterator directly, not just in @babel/runtime helpers.

@nicolo-ribaudo
Copy link
Member

We should force it to be included like we do for is-iterator:

const paths = ["is-iterable", "get-iterator"];

@taion
Copy link
Contributor Author

taion commented Mar 8, 2019

Makes sense. Done.

@nicolo-ribaudo nicolo-ribaudo merged commit dd2ffda into babel:master Mar 10, 2019
@taion taion deleted the symbol-async-iterator branch March 11, 2019 00:27
mAAdhaTTah added a commit to mAAdhaTTah/babel that referenced this pull request Mar 15, 2019
* master: (58 commits)
  Remove dependency on home-or-tmp package (babel#9678)
  [proposal-object-rest-spread] fix templateLiteral in extractNormalizedKeys (babel#9628)
  Partial application plugin (babel#9474)
  Private Static Class Methods (Stage 3) (babel#9446)
  gulp-uglify@3.0.2
  rollup@1.6.0
  eslint@5.15.1
  jest@24.5.0
  regexpu-core@4.5.4
  Remove input and length from state (babel#9646)
  Switch from rollup-stream to rollup and update deps (babel#9640)
  System modules - Hoist classes like other variables (babel#9639)
  fix: Don't transpile ES2018 symbol properties (babel#9650)
  Add WarningsToErrorsPlugin to webpack to avoid missing build problems on CI (babel#9647)
  Update regexpu-core dependency (babel#9642)
  Add placeholders support to @babel/types and @babel/generator (babel#9542)
  Generate plugins file
  Make babel-standalone an ESModule and enable flow (babel#9025)
  Reorganize token types and use a map for them (babel#9645)
  [TS] Allow context type annotation on getters/setters (babel#9641)
  ...
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue 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.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants