Allow rest/spread on polyfilled or builtin iterables without Symbol support
#11268
Conversation
nicolo-ribaudo
added
the
PR: Bug Fix 🐛
| if (!o) return; | ||
| if (typeof o === "string") return Array.from(o); | ||
| var n = Object.prototype.toString.call(o).slice(8, -1); | ||
| if (n === "Object" && o.constructor) n = o.constructor.name; |
There was a problem hiding this comment.
This should work with es6-shim. As an alternative, we could detect the presence of a _es6-shim iterator_ property.
I'm thinking specifically about es6-shim because it's the polyfill with most downloads on npm after core-js, and core-js's future is uncertain.
There was a problem hiding this comment.
By using Array.from, it already works with es6-shim without having to hardcode anything.
There was a problem hiding this comment.
Array.from is not supported on IE11. Can we use arrayLikeToArray?
There was a problem hiding this comment.
The problem is that Maps and Sets are not array-like.
I decided to use Array.from because because it is way more easily polyfilled than symbols.
WDYT about using arrayLikeToArray for strings/arguments/binary arrays, and Array.from for Map/Set?
There was a problem hiding this comment.
It's included with es6-shim tho, and imo it's completely reasonable to require anything faithfully polyfillable/shimmable, but symbols are not that.
@nicolo-ribaudo's suggestion works for me as well.
|
My |
|
@ljharb I would prefer to avoid loading them because they can't be easily inlined (when not using I think that we should see if this fix is "good enough", and if we get other bug reports we can iterate. |
…supported Currently, when `Symbol` is not supported, we allow using rest/spread with: - arrays - strings - arguments With this PR, it will be also possible to use it with - maps - sets - binary arrays While in old browsers es6 builtins would still need to be polyfilled, it's way easier to polyfill them because `Symbol` cannot be reliably polyfilled. I didn't use `instanceof` becase: - it doesn't work with polyfills not attatched to the global scope - when using Babel to load polyfills, it would force the inclusion of `Map` and `Set` polyfills even if they are not used Downside: the current approach of relying on `toString || construcor.name` doesn't work with subclasses.
nicolo-ribaudo
force-pushed
the
iterables-dx/map-set
branch
from
b2db406
to
4bbc468
Compare
nicolo-ribaudo
changed the base branch from
iterables-dx/rest-spread-better-error-messages
to
master
| if (typeof o === "string") return arrayLikeToArray(o, minLen); | ||
| var n = Object.prototype.toString.call(o).slice(8, -1); | ||
| if (n === "Object" && o.constructor) n = o.constructor.name; | ||
| if (n === "Map" || n === "Set") return Array.from(n); |
There was a problem hiding this comment.
@nicolo-ribaudo This should really be
if (n === "Map" || n === "Set") return Array.from(o);because n is a string!
github-actions
bot
added
the
outdated
Currently, when
Symbolis not supported, we allow using rest/spread with:With this PR, it will be also possible to use it with
While in old browsers es6 builtins would still need to be polyfilled, it's way
easier to polyfill them because
Symbolcannot be reliably polyfilled.I didn't use
instanceofbecause:MapandSetpolyfills even if they are not usedDownside: the current approach of relying on
toString || construcor.namedoesn't work with subclasses.Depends on #11264 (mostly to avoid merge conflicts, it shouldn't be a problem because the other PR is straightforward).
I'm marking this as a draft until that PR is merged, but this is ready for review.