montagejs / collections

This package contains JavaScript implementations of common data structures with idiomatic interfaces.
http://www.collectionsjs.com
Other
2.09k stars 183 forks source link

shim-array version of Array.from not compatible with object with length property #242

Closed sgronblo closed 4 years ago

sgronblo commented 4 years ago

The current shim-array version calls out to "Array.nativeFrom" in case the passed in value has a function in value[Symbol.iterator]

https://github.com/montagejs/collections/blob/master/shim-array.js#L30

If value[Symbol.iterator] is not a function it proceeds to call the array.addEach(values) instead which ignores the mapFn parameter. This breaks cases where Array.from is called like this: Array.from({length: 3}, (_, i) => i).

Why is the function checking for Symbol.iterator? Wouldn't it be better to just use nativeFrom if that is defined?

I know that there is a v2 branch in development for collections that is going to remove the shims. But there is no given timeline for its release so it might be quicker to make small patch releases to fix these broken shims in the meantime.

marchant commented 4 years ago

if(isSymbolDefined && values && (typeof values[Symbol.iterator] === "function" || typeof mapFn === "function")) { return Array.nativeFrom(values, mapFn, thisArg); }

Should do the trick. There collections using Array.from() passing a value that predated the native Array.from() that wouldn't be handled properly and create backward compatibility issues.

marchant commented 4 years ago

committed, give it a try

sgronblo commented 4 years ago

Yep, this would solve at least this particular instance of the problem I'm experiencing. Could you publish a new patched version with this change?

marchant commented 4 years ago

done