fix(rest-spread): Do not require Symbol.iterator for strings

[clshortfuse]

Q	A
Fixed Issues?	Fixes #9277
Patch: Bug Fix?	👍
Major: Breaking Change?	No
Minor: New Feature?	No
Tests Added + Pass?	Pass (with no tests added)
Documentation PR Link
Any Dependency Changes?	No
License	MIT

This applies the same logic as other runtime helpers by checking if typeof Symbol === 'function' before using Symbol.iterator as seen here:

babel/packages/babel-helpers/src/helpers.js

Line 178 in 3aaafae

if (typeof Symbol === "function" && Symbol.asyncIterator) {

The CoreJS2 version of this example would be compiled as:

if (typeof _Symbol === "function" && _Symbol$asyncIterator) {
  AsyncGenerator.prototype[_Symbol$asyncIterator] = function () {
    return this;
  };
}

You can also see this at

babel/packages/babel-helpers/src/helpers.js

Line 215 in 3aaafae

if (typeof Symbol === "function" && Symbol.iterator) {

So I can confirm the syntax is correct. I've tested on IE11 and spread now works properly (after adding a polyfill for Array.from).

Tested with:

function sum(x, y, z) {
  return x + y + z;
}

const numbers = [1, 2, 3];

console.log(sum(...numbers));
// expected output: 6

The only actual code change in packages/babel-helpers/src/helpers.js and everything else is auto generated by make bootstrap.

[babel-bot]

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

[zloirock]

It's should be fixed at least for working with arrays, now it should not work.

This approach will cause the same problems with Symbol injection in runtime and preset-env like the previous PR, it should be fixed. However, this approach looks better for me since it does not add special cases and injection of Map, Set, etc.

[clshortfuse]

@zloirock Do you have reference as to how it would produce an issue? Do you mean the conversion with corejs2? I did find other pieces of helper use typeof Symbol !== 'undefined which wouldn't care if Symbol() is available or not and then check Symbol.iterator. That would let Symbol be an object.

I did have another idea to solving this and that's ditching Symbol altogether. The reality is the code is checking to see if the Object has Symbol.iterator... and then never actually uses it. Unlike the other pieces of code that actually use Symbol.asyncIterator and Symbol.iterator, this one doesn't. If the code was going to call iter[Symbol.iterator]() then that's different.

In the end, it's just going to call Array.from and not use Symbol anyway. I think it makes for sense to just target what Array.from would actually do. Yes, all Objects that can be iterated with Symbol are arrays, but not that's exactly the best way to check for Array.from compatibility.

I was only targeting the regression specifically, but the alternative of Object.prototype.toString.call(iter) === "[object Arguments]" isn't that great either. For example, some ArrayLikes such as HTMLCollections can be used with Array.from but its prototype would be [object HTMLCollection]. Trying to target every single variation in some sort of whitelist is just asking for issues down the line (like Map or Set).

According to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/from#Polyfill the only thing that Array.from really needs is to check the following is Object(iter) != null. To be a bit more robust, we can check if iter.length > 0 || iter.length === 0. (It'll make sure it's a number and not null or NaN). I'm pretty sure Array.length can't be negative, but that can easily checked as well.

[zloirock]

@clshortfuse this line will add tens of kilobytes to the bundle and it's a serious regression.

It could be fixed on runtime and preset-env sides. But typeof checks of built-ins should not be just ignored - they should return or be replaced to related boolean.

In the end, it's just going to call Array.from and not use Symbol anyway.

I propose to use this approach in the loose mode - it keeps all problems to a minimum.

[clshortfuse]

I'm willing to dive in a bit more into the issue. I know sometimes solutions to problems tend to get shelved for lack of priority. I think finding the mechanics through which an Object can use Symbol.iterator can be a good goal.

Plainly stated, all the current code checks is whether the object is a candidate for Symbol iterator. We don't have to replicate the entirely Symbol functionality, but, instead, only it's check it's candidacy. (ie: if it can get an iterator, then it can receive an Array.from call).

Also, I think it's important to note the that the only purpose for the entire check is to simply cast (of sorts) an Object to an Array. As far as I've seen, there's no reference to this function outside of ToArray, so maybe taking a step back and looking at the original purpose may prove useful.

As for the mention of bloat, I'm assuming the issue lies with the importing of _Symbol from an external library. This is my first time playing around with a PR for Babel, so I wasn't aware of what consequences some functions would have. Still, I'll try to see what alternatives I can come up with. I'll probably need to dive deep into the ECMA spec and review a handful of popular Symbol polyfills just to ensure we don't cause any conflicts, and ultimately, hopefully it can be used outside of loose mode.

[clshortfuse]

I took another stab at this. I took another approach based on how iterableToArrayLimit attempts to use Symbol.iterator

babel/packages/babel-helpers/src/helpers.js

Line 891 in 3aaafae

} catch (err) {

Basically, it's wrapped in a try block. If it fails, then it discards the error and tries the prototype. If that doesn't success, toArray is going to call _nonIterableRest() which throws a TypeError.

babel/packages/babel-helpers/src/helpers.js

Line 924 in 3aaafae

throw new TypeError("Invalid attempt to destructure non-iterable instance");

Doing it this way instead of how v6 was doing it (checking prototype first) avoids the performance penalty of the string comparison in contexts that support Symbol.iterator. I broke out the prototype whitelist into an array, so we can change it in the future to:

  const prototypeWhiteList = [
      '[object Arguments]',
      '[object HTMLCollection]',
      '[object NamedNodeMap]',
      '[object Map]',
      '[object Set]',
    ];

Out of curiosity, and not exactly related to the issue, but if we call (Array.prototype.from || [].slice).call(iter) we can then remove any requirements for IE11. It would only serve as a fallback. I'm sure there's some specific reason why we can't do this now (probably core-js), but it would be nice to have.

[ljharb]

Why is a try/catch needed here instead of just checking if Symbol.iterator exists or not?

[clshortfuse]

You'd have to check if Symbol exists before checking if Symbol.iterator exists. I squashed the previous commit, but checking if Symbol exists would bloat the code up, as explained the one of the previous comments.

Doing it this way removes the need for typeof checks and lets the browser pass the Symbol check failure to catch. Since we're not actually using Symbol.iterator the result would be same. If it can't check the object to see if it has it, then it wouldn't have it anyway.

[ljharb]

try/catch has a performance penalty that's far worse than a few extra gzippable bytes.

[clshortfuse]

As I mentioned in the other comment, this is the fourth try in the same helper file. I don't see why this specific one should be treated any differently.

Edit: 4th try, 2nd specific to Symbol

[ljharb]

because in that case, it's being used to catch errors thrown by user code (the iterator method). In this case it's used to catch errors in your code itself (attempting unchecked member access on a maybe-not-object).

[clshortfuse]

Ah, in the next() function. Okay, back to the drawing board.

[clshortfuse]

According to ES6 spec, the following prototypes use @@iterator

• String
• Array
• Map
• Set

WeakMap isn't guaranteed to have an iterator. TypedArray is listed as well, but since it looks like a native abstract type, I couldn't find a way to target all of them. I removed Map and Set since it would add more imports in corejs2. It doesn't seems possible to really check if an object uses Symbol.iterator without actually checking for Symbol and iterator. I did have another method of checking for Symbol, namely:

const env = ((typeof window !== 'undefined') && window)
  || ((typeof global !== 'undefined') && global)
  || ((typeof self !== 'undefined') && self);
if (!env || 'Symbol' in env) {
  if (Symbol.iterator in iterObject) {
    return Array.from(iter);
  }
}

Since I'm only targeting fixing the V6 regression with this PR, I resorted to leaving it out.

[jridgewell]

Array.isArray

[clshortfuse]

Array.isArray isn't the same a prototype check. For example, Array.isArray would return true when checked against Array.prototype where this wouldn't. Also, Array.isArray is already checked prior to this function.

[jridgewell]

Wasn't the point of this PR to do a typeof Symbol !== 'undefined' check?

[clshortfuse]

No. It's to fix the spread regression specifically.

[jridgewell]

But none of your changes will help spreading the HTMLCollection from the issue: [...document.getElementsByTagName('div')]. I don't understand what you're looking for then.

[clshortfuse]

The input code shows the error, but doesn't accurately detail the regession. It's in the description, which is more accurate. Basically, all spread functions fail on IE11 because it's checking for Symbol before checking the prototype. It's a regression from v6. I believe it's caused by 4da3f3b

[jridgewell]

The code in the OP goes through arrayWithHoles's Array.isArray check, so this change does nothing still. I need an explicit "this fails in IE 11, doesn't fail with this change" case so that I can review the PR.

[clshortfuse]

I think you're missing the point of the [object Arguments] check. That block never executes on environments without Symbol. Therefore any function with spread arguments would fail.

function func1(...args) {
  console.log(args[0]);
  // expected output: 1

  console.log(args[1]);
  // expected output: 2

  console.log(args[2]);
  // expected output: 3
}

func1(1, 2, 3);

Edit: Forgot the spread!

[clshortfuse]

Looking back at the issue from 3 months ago, the request was a partial fix for the regression while the more complicated issue of completely eliminating Symbol is explored. That PR has been stuck for a couple of months now.

I think that's why I had the mindset of swapping the order of execution, at least to minimize the issue.

[jridgewell]

So, this is the kind of thing you're trying to fix? https://babeljs.io/repl/build/master#?babili=false&browsers=ie%20%3E%2010&build=&builtIns=false&spec=false&loose=false&code_lz=CYUwNiAuIAQO4EsB2wD2cB0BlAngWwCNUwBuAKADMBXJAY0gVSRgGcq8AKADwBoYc-ALwCUMAN5kYMAE5Qq05lxgBqfipiDyAX0o16jZtBaQOoiVIBuAQ2kwbAcxYwAvHen32IJJBbkpsyHlmNk4MMIcWYW0yMlomFmIQDDBUew4jEwBGPgAmPgBmYSigA&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=true&fileSize=false&timeTravel=false&sourceType=module&lineWrap=true&presets=env&prettier=false&targets=&version=7.4.3&externalPlugins=

A executable test case we can add is:

function sum(x, y, z) {
  return x + y + z;
}
function test() {
  var args = arguments;
  return sum(...args);
}

assert.equal(test(1, 2, 3), 6);

All that's necessary for this was to change the order of the || expression in the old code.

[clshortfuse]

Yes, and wrap them in parentheses. Also, any object that is a prototype of Array and String would align with the ES6 spec.

I'm not seeing much more we can do that wouldn't trigger extra corejs2 imports. I have no way of checking for the internal TypedArray type. Also, NodeList, HTMLCollection and others aren't part of the ES6 spec. I realized that this function, at least as it's called, is not checking for array-likes. It's checking for iterables. Therefore, something like this:

[...({ 0: 'a', 1: 'b', 2: 'c', length: 3})]

would throw an error on an ES6 supporting browser, and therefore should throw an error on Babel. Support for array-likes would have to be loose. I think this the best we can do without violating spec.

[jridgewell]

We can add a string check, that's fine. The array check is already covered by arrayWithHoles (the difference between Array.prototype passing Array.isArray and not Array.prototype.isPrototypeOf(Array.prototype) doesn't matter). Let's do the string check, then arguments check, then isIterable check.

[clshortfuse]

Okay, I've made the changes to this code. I also changed the commit and topic to better reflect the limited scope of this fix. I'm only targeting the rest parameter regression here.

From my research, I've hit a bit of a wall getting a catch-all solution. I've come to agree with @zloirock that the loose option is probably the only possible way to make it all work, but it definitely wouldn't be to ES6 spec. For example, on a real ES6 browser, this:

[...({ 0: 'a', 1: 'b', 2: 'c', length: 3})]

would throw an non-iterable error. That means babel should as well. It would be bad if babel would run something fine when transpiled to ES5, and then if somebody makes a ES6 version without babel, realizes that it doesn't work. That's because spread only works for iterables, NOT array-likes. Even though Array.from would accept it, it would be wrong to do so. Therefore any checks based on .length wouldn't be to spec.

There's more that can be done to target Map and Set which are part of the ES6 spec, but in my opinion, that would require changes to how typeof is transpiled. They should be checked without corejs2 importing the classes. Perhaps a special $isUndefined() could help in this case. Regardless, I think the best way to attack this problem is piece by piece and this is just a partial fix. We can explore that in another PR (I also have a couple of other theories as to checking if an object is using Array.prototype.values() as @@iterator).

Lastly, I want to apologize to the devs for the possibly annoying back and forth with the changes due to not fundamentally understanding the true underlying nature of @@iterator in ES6.

Note to @jridgewell : The reason why I didn't put the String check outside iterableToArray is because I wasn't sure if I should put it with all functions that call it. I felt it was safer to keep it inside.

[clshortfuse]

If I add

// Do not add import if only checking typeof
if (t.isUnaryExpression(parent, { operator: "typeof" })) return;

to

babel/packages/babel-plugin-transform-runtime/src/index.js

Line 264 in 3aaafae

Then I can add the following without forcing imports:

if (typeof Symbol !== 'undefined' && (Symbol.iterator in Object(iter))) {
  return Array.from(iter);
}

Please advise to do so, or just keep this PR as is.

Edit: Though taking another look, that would cause a false positive. The reality is that we probably should let the compiler throw an error saying Symbol is not found. We wouldn't want to incorrectly let the function pass void, which throws a TypeError. The user could read the error and assume that ... isn't compatible with the object in question. We actually don't know if it's not compatible because Symbol isn't available and the error shouldn't be a false one.

[jridgewell]

Please add an exec.js test from #9794 (comment)

[clshortfuse]

@jridgewell Let me know when you want me to squash the commits.

[jridgewell]

No need, we can squash merge them on GitHub.

[nicolo-ribaudo]

We will fix this to also work with Map/Set without loading the polyfills from core-js, but thank you for at least fixing this case!

[zloirock]

I don't see any sense in this PR in the current form since without Symbol.iterator will work only arguments and strings, but even they require Array.from. Spread will not work even with arrays without Symbol.iterator.

[clshortfuse]

@zloirock I don't understand. You rather it be in a more broken state?

It's a regression. There is no catch-all fix that I've seen without polyfilling Symbol. If we can at least partially resolve the regression, then people don't have to change perfectly working V6 code around.

Edit:

Spread will not work even with arrays without Symbol.iterator.

I'm pretty sure the function that is called before this one checks Array.isArray, so you don't need Symbol.Iterator for arrays, you just need Array.from support.

[zloirock]

@clshortfuse I'm not against of adding support of work spread without Symbol. iterator to babel@7, however I don't think that the current behavior is a regression - it's a breaking change in a major release, but by my vision, it's available since it makes babel behavior more correct by the spec. However, OK, let babel@7's spread work even without Symbol.iterator. I don't understand what's the heck is going on in this PR, see my comment above - those changes are almost completely useless since it does not support even arrays.

[zloirock]

Ok, my vision of this helper for babel@7:

function iterableToArray(it) {
  switch (Object.prototype.toString.call(it).slice(8, -1)) {
    // As an option, they will work even without `Array.from` polyfill,
    // however, it could cause problems with custom iterators:
    case "Arguments":      // It will not work in old IE without `core-js`
    case "HTMLCollection": // 2 most popular iterable DOM collections,
    case "NodeList":       // for work with other you should load `core-js`
      return Array.prototype.slice.call(it);
    case "String":         // Since of code units, we should use `Array.from`
    case "Set":            // It will work with `es6-shim` if they will add
    case "Map":            // internal @@toStringTag support
      return Array.from(it);
  }
  if (it == null) return;
  // It's more correct by the spec than `in` check
  // However, it will not work with `@babel/runtime-corejs2` -/
  const iterator = it[Symbol.iterator];
  if (typeof iterator !== 'function') return;
  return Array.from({ [Symbol.iterator]: iterator.bind(it) });
}

[clshortfuse]

@zloirock, I originally had an array prototype check but we agreed to remove it since the previous calling function is going to call Array.isArray.

I have my old commits here. I did have a list of every ES6 type in spec. I decided against the typeof check, before calling Set, Map or Symbol because it would lead to the wrong error. The user should get notified they don't exist and not a TypeError. That would be a spec violation. It can be confusing to give the user the incorrect error and we don't want people to think the object they are using spread against is an invalid one.

I opted out of non ES6 prototypes like HTMLCollection because they're not part of the actual spec. It's stricter, but whitelisting would be too loose, IMO, and go against the goal of being to spec.

I'm being pretty strict to limiting to just what ES6 has with String and rest arguments. Anything else should throw the correct error if it's not available. If users don't want it to be so strict, we can improve the loose mode.

[zloirock]

Ok, seems I missed a moment about Array.isArray in arrayWithoutHoles -> toConsumableArray, however, this PR still makes spread work only on arguments and strings and only with polyfilled Array.from, which is not really useful. As you can see in the example of the helper above, you can unobtrusively check the argument on internal classes without type/reference errors and loading unnecessary polyfills. Iterators for DOM collections were added to core-js only because of a babel's feature request, they are added in @babel/polyfill, @babel/runtime, @babel-preset-env, so adding them in those cases and not adding to special cases of this helper (if they are will be added) are double standards.