Fix map spreading

[quisido]

Getting this error in ESLint, which seems perfectly valid:

Invalid attempt to spread non-iterable instance.
In order to be iterable, non-array objects must have a [Symbol.iterator]() method.
Referenced from: .../.eslintrc.cjs
    at _nonIterableSpread (.../.yarn/cache/@babel-runtime-npm-7.18.6-e238904aef-8b707b64ae.zip/node_modules/@babel/runtime/helpers/nonIterableSpread.js:2:9)
    at _toConsumableArray (.../.yarn/cache/@babel-runtime-npm-7.18.6-e238904aef-8b707b64ae.zip/node_modules/@babel/runtime/helpers/toConsumableArray.js:10:95)
    at Object.<anonymous> (.../.yarn/__virtual__/eslint-plugin-jsx-a11y-virtual-70ad45934f/7/.../.yarn/cache/eslint-plugin-jsx-a11y-npm-6.6.0-d57094bb84-d9da9a3ec7.zip/node_modules/eslint-plugin-jsx-a11y/lib/util/isInteractiveElement.js:24:61)

isInteractiveElement has a couple of instances of [...someMap], where the map is not a literal Map object, but instead is a vanilla object with a couple of hand-written Map prototype methods.

This package then uses Babel to transpile the spread operator, and that very Babel transpilation throws an error that said object cannot be spread.

An alternative solution could be to add the [Symbol.iterator]() method to the faux maps, but this seems more explicitly accurate and faster than nested dependency changes. I used entries() because [...new Map()] returns an array of the map's entries.

[codecov]

Codecov Report

Merging #875 (27de267) into main (0d2bc43) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #875      +/-   ##
==========================================
+ Coverage   99.26%   99.29%   +0.03%     
==========================================
  Files          98      101       +3     
  Lines        1488     1555      +67     
  Branches      492      511      +19     
==========================================
+ Hits         1477     1544      +67     
  Misses         11       11              

Impacted Files	Coverage Δ
src/util/isInteractiveElement.js	100.00% <100.00%> (ø)
src/util/getAccessibleChildText.js	100.00% <0.00%> (ø)
src/rules/anchor-ambiguous-text.js	100.00% <0.00%> (ø)
src/rules/prefer-tag-over-role.js	100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[ljharb]

I'm confused - is your babel configured to transpile node_modules? It's never safe to transform code you didn't author.

[ljharb]

While I agree this is correct (as is the existing code), I'm confused why it's not working for you, since it's been published this way for awhile.

[ljharb]

hm, i also note you're using yarn pnp, which breaks a bunch of things. Does this happen using node's actual resolution mechanism?

[quisido]

I'm confused - is your babel configured to transpile node_modules? It's never safe to transform code you didn't author.

@ljharb This is not related to the consumer's Babel configuration. The references to Babel in the description refer to this package's own build configuration.

While I agree this is correct (as is the existing code), I'm confused why it's not working for you, since it's been published this way for awhile.

I wish I knew too. It works when executing eslint by command line, but it fails when executing eslint via the VS Code editor. Frankly, I'm more concerned that the code works for anyone, since the error seems completely accurate. 😂 I read the code, and there's no reason this error shouldn't be thrown 100% of the time. It essentially says, "If it's not Array.isArray, instanceof Map, instanceof Set, or have a [Symbol.iterator] method, then throw an error." It definitely doesn't meet any of the criteria, so the error should always be thrown.

I patched my local copy to use .entries() and verified that the error does go away once the update is applied.

hm, i also note you're using yarn pnp, which breaks a bunch of things. Does this happen using node's actual resolution mechanism?

I don't believe it's Yarn PNP, because yarn eslint works as expected. I actually think it may be the Yarn VS Code SDK for ESLint. I don't know why it exclusively would result in this error, because as stated, the error is accurate.

Node is not explicitly installed on the machine that encounters this error, so while yarn eslint uses Node (and passes), the VS Code SDK must be using whatever JS runtime that VS Code itself uses. Electron? Which is probably Node behind-the-scenes? It must be a version of Node that doesn't support this. 🤷‍♂️

I think the most important thing is that this fix is accurate. 👍 If it had to be defined, "Adds support for Electron."

[ljharb]

yarn eslint uses PnP, to try it without, you'd need to do npx eslint or ./node_modules/.bin/eslint, but that won't work unless you've installed without PnP.

[ljharb]

I agree the fix is accurate; what's mystifying is how you're the first person to report the bug. Additionally, I'm not able to reproduce it - which is why i suspect it might be related to PnP. What version of @babel/runtime do you have installed?

[quisido]

I don't believe it is a PNP issue because it is working with PNP when I execute it by CLI. It only doesn't work via the Yarn ESLint SDK for VSCode.

I can't confirm the Babel runtime version until Monday, as this occurred on a work device, and I don't want to work before then. 😆

[ljharb]

Right - but that's what I'm saying - typically when using PnP, lots of integrations tend to break.

Happy to wait :-) it's important to understand why something is broken before fixing it.

[quisido]

I'm not sure I align with that perspective. It's broken because it's trying to spread a non-spreadable object. It's clear why it's broken. The part that doesn't make sense is why it works. I do not believe that we first need to understand why it works before fixing why it's broken. This change is backwards compatible.

[ljharb]

My hunch is that you're using a version of @babel/runtime that has different behavior - ie, that throws in this case, whereas the latest version of babel does not.

[quisido]

@babel/runtime is the latest version: 7.18.6.

The Babel file throwing the error hasn't been changed since November 2021.

Investigation steps:

// eslint-plugin-jsx-a11y/lib/util/isInteractiveElement.js
12: var _toConsumableArray2 = _interopRequireDefault(require("@babel/runtime/helpers/toConsumableArray"));
24: var elementRoleEntries = (0, _toConsumableArray2["default"])(_ariaQuery.elementRoles);

toConsumableArray is throwing an error when it receives elementRoles (which is a vanilla object, shaped { entries(){}, keys(){}, values(){}, etc }).

// @babel/runtime/helpers/toConsumableArray.js
01: var arrayWithoutHoles = require("./arrayWithoutHoles.js");
03: var iterableToArray = require("./iterableToArray.js");
05: var unsupportedIterableToArray = require("./unsupportedIterableToArray.js");
07: var nonIterableSpread = require("./nonIterableSpread.js");
09: function _toConsumableArray(arr) {
10:  return arrayWithoutHoles(arr) || iterableToArray(arr) || unsupportedIterableToArray(arr) || nonIterableSpread();
11: }

toConsumableArray checks if the value is an "array without holes" (Array.isArray), an "iterable" (has a Symbol.iterator key), an "unsupported iterable" (Map or Set), or else it throws an error (nonIterableSpread is a function that only throws an error):

I don't know why the file toConsumableArray.js is toArray.js in the Babel repo, but the contents are verbatim the same. You should be able to verify these in your local node_modules.

I also found this issue where the maintainers state that plain objects are not iterable and are expected to throw an error when passed to toConsumableArray.

[quisido]

Sorry, I closed this because I discovered the reason this wasn't impacting others. When I patched eslint-plugin-jsx-a11y to contain aria-query as a peerDependency, I had the wrong version.

This package depends on aria-query@^4, whereas I was using aria-query@^5.

If you want to keep this PR, you should be able to go ahead and bump aria-query to ^5 (and should include it as a peer dependency for Yarn berry support).

[ljharb]

ahhh, thanks for clarifying. in that case yes, i'll close this as a duplicate of #814.