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

shim-array find() doesn't use the same params as the canonical one. #182

Closed
Jorenm opened this issue May 5, 2017 · 7 comments
Closed

shim-array find() doesn't use the same params as the canonical one. #182

Jorenm opened this issue May 5, 2017 · 7 comments
Assignees
Milestone

Comments

@Jorenm
Copy link

Jorenm commented May 5, 2017

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find?v=control

Also, shouldn't a shim check if it exists before it replaces it?

This took up maybe 5 hours of debugging till I dug into the right node_module to find its functionality broken because it expected the canonical .find functionality :(

@uglow
Copy link

uglow commented Jun 22, 2017

Here's the dependency tree that I had that ended up leading me here:

├─┬ dgeni-packages@0.16.1
│ ├─┬ catharsis@0.8.8
│ │ └─┬ underscore-contrib@0.3.0
│ │   └── underscore@1.6.0
│ ├─┬ change-case@3.0.0
│ │ ├── camel-case@3.0.0
│ │ ├── constant-case@2.0.0
│ │ ├── dot-case@2.1.1
│ │ ├── header-case@1.0.1
│ │ ├── is-lower-case@1.1.3
│ │ ├── is-upper-case@1.1.2
│ │ ├── lower-case@1.1.4
│ │ ├── lower-case-first@1.0.2
│ │ ├── no-case@2.3.1
│ │ ├── param-case@2.1.1
│ │ ├── pascal-case@2.0.1
│ │ ├── path-case@2.1.1
│ │ ├── sentence-case@2.1.1
│ │ ├── snake-case@2.1.0
│ │ ├── swap-case@1.1.2
│ │ ├── title-case@2.1.1
│ │ ├── upper-case@1.1.3
│ │ └── upper-case-first@1.1.2
│ ├── espree@2.2.5
│ ├── estraverse@4.2.0
│ ├─┬ glob@7.1.2
│ │ ├── fs.realpath@1.0.0
│ │ └── inflight@1.0.6
│ ├─┬ htmlparser2@3.9.2
│ │ ├── domelementtype@1.3.0
│ │ ├── domhandler@2.4.1
│ │ ├─┬ domutils@1.6.2
│ │ │ └─┬ dom-serializer@0.1.0
│ │ │   └── domelementtype@1.1.3
│ │ ├── entities@1.1.1
│ │ └─┬ readable-stream@2.2.11
│ │   ├── core-util-is@1.0.2
│ │   ├── process-nextick-args@1.0.7
│ │   ├── safe-buffer@5.0.1
│ │   ├── string_decoder@1.0.2
│ │   └── util-deprecate@1.0.2
│ ├── marked@0.3.6
│ ├─┬ minimatch@3.0.4
│ │ └─┬ brace-expansion@1.1.8
│ │   ├── balanced-match@1.0.0
│ │   └── concat-map@0.0.1
│ ├── node-html-encoder@0.0.2
│ ├── nunjucks@2.5.2
│ ├─┬ q-io@1.13.4
│ │ ├─┬ collections@0.2.2          <----
│ │ │ └── weak-map@1.0.0
│ │ ├── mime@1.3.6
│ │ ├── mimeparse@0.1.4
│ │ ├── qs@6.4.0
│ │ └── url2@0.0.0

Even though this is an old version of collections, @Jorenm 's feedback is valid.

@jh3141
Copy link

jh3141 commented Oct 22, 2017

+1. Changing existing standardised behavior of core objects is a very bad idea, particularly if the change is documented as not happening. The documentation states:

In version 1, this method is called find, which conflicts with the definition of find provided by ECMAScript 6. In version 2, this method is called findValue to eliminate the conflict.

As I have the most current version available in NPM (5.0.7, according to my package.json file), I would have assumed that I'd get the version 2 behaviour, but apparently it has been reverted to the behaviour of version 1 for some reason.

@mattandrews
Copy link

I too have just had several issues due to this library's poorly-named functions – with great power comes great responsibility(!).

@hthetiot hthetiot added this to the 5.0.x milestone Dec 6, 2017
@hthetiot hthetiot added the bug label Dec 6, 2017
@hthetiot
Copy link
Contributor

hthetiot commented Dec 6, 2017

@mattandrews Can you try npm install collections@2.0.3 and tell me if that fix your issue.
I'm going to merge v2 branch in master and release v6 soon.

V3 to v5 was released with legacy shim and find override instead of findValue.

@hthetiot
Copy link
Contributor

hthetiot commented Dec 7, 2017

Duplicate #185

@hthetiot hthetiot self-assigned this Dec 7, 2017
@hthetiot hthetiot closed this as completed Dec 7, 2017
@mattandrews
Copy link

Hi, thanks for the update. Unfortunately my issue was with a library several steps upstream from collections, so even if the issue is fixed here I need to get two further projects to update their dependencies (eg. see the explanation here).

Since commenting here I migrated my code away from the original tools I was trying to use as the effect this was having on my codebase was unmanageable (eg. uses of Array.find() within my app were broken).

@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. As mentioned, branch v2 fixes these issues by avoiding global object modification.

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

6 participants