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

Array.prototype.find() returns -1 instead of undefined #178

Closed
theetrain opened this issue Apr 24, 2017 · 6 comments
Closed

Array.prototype.find() returns -1 instead of undefined #178

theetrain opened this issue Apr 24, 2017 · 6 comments

Comments

@theetrain
Copy link

The default behaviour of Array.prototype.find is to return the first matching value or undefined if not found. Collections.js returns -1 instead of undefined, which causes an unexpected issue with truthiness and coercion:

Standard behaviour:

[].find(a => true) // undefined
!![].find(a => true) // false

Collections.js behaviour:

[].find(a => true) // -1
!![].find(a => true) // true

I'm only curious why this decision was made.

@kriskowal
Copy link
Member

I wrote Collections before the ink dried on the spec. Versions 1 and 3+ seem to be stuck with my mistake. @asolove and I fixed it in version 2.

This comes up often enough that @Stuk and I are considering forking collections and making an @collections/ org in npm rooted on version 2. That involves a lot of work and it is not on my evening and weekend roadmap. If there are people interested in sinking effort in that project, let us know. It would be great to have a community around such a central concern.

Collections is a Montage project so compatibility with Montage and all its inertia are the primary concern that dictates that Collections never stray far from backward compatibility with a host of API mispredictions.

@kennethklee
Copy link

why not, not modify native Array.prototype. This destroys any optimisations JS can do. Rather, create a new class based off Array.

@kriskowal
Copy link
Member

@kennethklee as mentioned, Version 2 does not monkey-patch Array.prototype at all.

@kriskowal
Copy link
Member

Alright, this has become enough of a problem that we will need broader support for collections in general. Please drop a line in here if you’ve got cycles to help move collections for JavaScript into a community supported suite to get around Montage’s compatibility limitations. We can talk about breaking the collections package into an @collections organization and how we can build a group of maintainers to make that migration.

https://docs.google.com/forms/d/e/1FAIpQLSd4j9-Uwy4Yoc9djU9Yh6RyhucjDhoyG58N4Fck7OZgHAtL-Q/viewform?c=0&w=1

@hthetiot
Copy link
Contributor

hthetiot commented Dec 7, 2017

#185

@codebling
Copy link

codebling commented Apr 15, 2019

See also issues #36 #70 #94 #95 #116 #139 #145 #162 #165 #169 #178 #182 #185 #197 #215 #220 and PRs #94 #95 #116 #173 #189 #212. Branch v2 fixes these issues by avoiding global object modification.

basti1302 added a commit to instana/nodejs that referenced this issue Apr 24, 2020
- Make instrumentation robust against montagejs/collections#178.
- Make check for disabled instrumentation stricter (the keys provided in config.tracing.disabledTracers/INSTANA_DISABLED_TRACERS need to be an exact, case-insensitive match of the instrumentation file now). If you have used this configuration option and relied on the fact that this was a string.contains check previously, you might need to update your config.
basti1302 added a commit to instana/nodejs that referenced this issue Apr 24, 2020
- Make instrumentation robust against montagejs/collections#178.
- Make check for disabled instrumentation stricter (the keys provided in config.tracing.disabledTracers/INSTANA_DISABLED_TRACERS need to be an exact, case-insensitive match of the instrumentation file now). If you have used this configuration option and relied on the fact that this was a string.contains check previously, you might need to update your config.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants