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: early return when instance is not iterable #10396

Merged
merged 3 commits into from Sep 6, 2019

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Sep 5, 2019

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

The slicedToArray is supposed to throw nonIterableRest error when applied to an non-iterable instance. However, the iterableToArrayLimit will always throw error in arr[Symbol.iterator] when arr is not iterable.

Here we early return in iterableToArrayLimit so that we can offer better error message in runtime. Note that by doing so we also align iterableToArrayLimit with iterableToArray.

@@ -943,7 +943,7 @@ helpers.iterableToArrayLimit = helper("7.0.0-beta.0")`
// _e = _iteratorError
// _i = _iterator
// _s = _step

if (!Symbol.iterator in Object(arr)) return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The iterableToArray has guarded this check with an arguments check

export default function _iterableToArray(iter) {
    if (
      Symbol.iterator in Object(iter) ||
      Object.prototype.toString.call(iter) === "[object Arguments]"
    ) return Array.from(iter);
  }

But

(function () { return Symbol.iterator in Object(arguments) })()

returns true in Chrome 76. I am not sure if this behavior is inconsistent across different browsers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, there is a browser support matrix on arguments[@@iterator]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/arguments/@@iterator

@JLHwung JLHwung added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Sep 5, 2019
@babel-bot
Copy link
Collaborator

babel-bot commented Sep 5, 2019

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

@babel-bot
Copy link
Collaborator

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

nicolo-ribaudo
nicolo-ribaudo previously approved these changes Sep 5, 2019
@nicolo-ribaudo nicolo-ribaudo dismissed their stale review September 5, 2019 23:38

Actually yes, we need to check for arguments. This is because arguments can't be polyfilled (becaue it is re-created every time), so Symbol.iterator in Object(arguments) will always be false in old browsers, even with a polyfill.

@nicolo-ribaudo nicolo-ribaudo merged commit b64cb9a into babel:master Sep 6, 2019
waynee95 added a commit to waynee95/mv-plugins that referenced this pull request Nov 15, 2019
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Dec 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 6, 2019
@JLHwung JLHwung deleted the fix-9210 branch December 15, 2020 15:42
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 PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: undefined is not iterable
4 participants