Array#find is implemented wrong #139
Comments
|
These issues are addressed in version 2 and can be installed via npm On Mon, Jan 4, 2016 at 6:27 PM Kyle Hardgrave notifications@github.com
|
|
But, why add the wrong |
there was a bug with Array.find that was giving us trouble (bug: montagejs/collections#139) and this bug is no longer present in 2.0 version of collections.
This fixes this problem:
[2016-01-20 17:58:43.560] [ERROR] demo-job-manager - child-process-error Unhandled rejection TypeError: serializer.serialize is not a function
at PluggableJSON._serialize (/Users/mstemm/work/src/outrigger/node_modules/juttle-jsdp/node_modules/pluggable-json/lib/index.js:64:35)
at PluggableJSON.serialize (/Users/mstemm/work/src/outrigger/node_modules/juttle-jsdp/node_modules/pluggable-json/lib/index.js:53:41)
at Object.serialize (/Users/mstemm/work/src/outrigger/node_modules/juttle-jsdp/lib/protocol/index.js:21:30)
at send (/Users/mstemm/work/src/outrigger/lib/juttle-subprocess.js:61:23)
at /Users/mstemm/work/src/outrigger/lib/juttle-subprocess.js:177:13
And was related to this:
montagejs/collections#139
This fixes this problem:
[2016-01-20 17:58:43.560] [ERROR] demo-job-manager - child-process-error Unhandled rejection TypeError: serializer.serialize is not a function
at PluggableJSON._serialize (/Users/mstemm/work/src/outrigger/node_modules/juttle-jsdp/node_modules/pluggable-json/lib/index.js:64:35)
at PluggableJSON.serialize (/Users/mstemm/work/src/outrigger/node_modules/juttle-jsdp/node_modules/pluggable-json/lib/index.js:53:41)
at Object.serialize (/Users/mstemm/work/src/outrigger/node_modules/juttle-jsdp/lib/protocol/index.js:21:30)
at send (/Users/mstemm/work/src/outrigger/lib/juttle-subprocess.js:61:23)
at /Users/mstemm/work/src/outrigger/lib/juttle-subprocess.js:177:13
And was related to this:
montagejs/collections#139
|
@kriskowal, do you have an answer to @morlay's question? Thanks. |
|
Collections are managed by Montage Studio. They needed to cut a major version that fixed other things, but retained this old behavior for compatibility with deployed versions in production. So version 2 is still ahead of version 3 in this regard. Montage was not able to use version 2 because I made radical changes to the observer subsystem, factoring it into a library and changing the interface. Montage will not be able to reunite with the changes I made for version 2 until someone upgrades FRB to use version 2 change observers. This is not on my roadmap. |
|
However, Version 4 could cherry-pick the fixes in version 2 for this feature with minimal impact to Montage applications and without breaking FRB. |
|
ok, thanks. v2 it is. |
|
For issues and pulls against v2, I maintain v2 as the master branch on my fork https://github.com/kriskowal/collections. Thanks. |
|
Is there any way we can add a deprecated warning or something on this. It caused a production bug for us as well. |
|
I think a deprecation warning would be reasonable, a log to console like “Warning: collections v1 monkey-patches the find method in a way inconsistent with the ECMAScript 6 specification.” What the warning suggests you do is the hard part. v2 is not eligible for folks using Montage. v3 does not solve this problem. v2 in fact, no longer monkey-patches Array at all, and instead favors polymorphic functions like I’ll defer to @marchant to sort out the deprecation and migration story. |
This method is injected in the `collections`-package as array-shim, which is a dependency of q-io. Version 1.x of `collections` implements the method with the semantics of `Array.prototype.findIndex` instead. This leads to problems with other packages that rely on the implementation as defined in the ES6-Standard See also: montagejs/collections#139
This method is injected in the `collections`-package as array-shim, which is a dependency of q-io. Version 1.x of `collections` implements the method with the semantics of `Array.prototype.findIndex` instead. This leads to problems with other packages that rely on the implementation as defined in the ES6-Standard See also: https://github.com/montagejs/collections/issues/139Use q-io@2 to exclude wrongly implemented "Array.prototype.find"-method This method is injected in the `collections`-package as array-shim, which is a dependency of q-io. Version 1.x of `collections` implements the method with the semantics of `Array.prototype.findIndex` instead. This leads to problems with other packages that rely on the implementation as defined in the ES6-Standard See also: montagejs/collections#139
…otype.find" This method is injected in the `collections`-package as array-shim, which is a dependency of q-io. Version 1.x of `collections` implements the method with the semantics of `Array.prototype.findIndex` instead. This leads to problems with other packages that rely on the implementation as defined in the ES6-Standard See also: https://github.com/montagejs/collections/issues/139Use q-io@2 to exclude wrongly implemented "Array.prototype.find"-method This method is injected in the `collections`-package as array-shim, which is a dependency of q-io. Version 1.x of `collections` implements the method with the semantics of `Array.prototype.findIndex` instead. This leads to problems with other packages that rely on the implementation as defined in the ES6-Standard See also: montagejs/collections#139 Zeilen,
|
Duplicate #185 |
Your implementation of
Array.prototype.find()is wrong — it returns the index of the matching element, not the element itself. See the MDN article on.find()— you've implemented.findIndex.Relatedly, it's very upsetting that this library doesn't bother to check
Array.prototypefor a native version first before overwriting. This ruins the party for anyone who consumes this library as a node module, even if it's only as a second-order dependency.The text was updated successfully, but these errors were encountered: