Skip to content

use _.contains in _.uniq instead of indexOf #1868

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

Merged
merged 1 commit into from
Sep 23, 2014

Conversation

akre54
Copy link
Collaborator

@akre54 akre54 commented Sep 23, 2014

No description provided.

michaelficarra added a commit that referenced this pull request Sep 23, 2014
use _.contains in _.uniq instead of indexOf
@michaelficarra michaelficarra merged commit 33812ac into jashkenas:master Sep 23, 2014
@megawac
Copy link
Collaborator

megawac commented Sep 23, 2014

This was changed for a slight perf win a while ago - there's already a jsperf somewhere

@akre54
Copy link
Collaborator Author

akre54 commented Sep 23, 2014

But it's the only place that uses it. Either we use one or the other
throughout the source

@akre54
Copy link
Collaborator Author

akre54 commented Sep 23, 2014

Plus it looks like the uniq can be refactored a bit more from here

@megawac
Copy link
Collaborator

megawac commented Sep 23, 2014

@akre54 smt like this is another option 2ce6f62

@akre54 akre54 deleted the uniq-contains branch September 23, 2014 17:07
@akre54
Copy link
Collaborator Author

akre54 commented Sep 23, 2014

Yeah, though if we're going to go that route we should look into #1862 too. I'm not sure underscore is the place (yet) for es6 shims, but probably will be soon. Given that most of these features are still behind flags lets give them some time to mature.

@jdalton
Copy link
Contributor

jdalton commented Sep 23, 2014

It might be a nice addition to Underscore in the future, but given that most of these features are still behind flags lets give them some time to mature.

Things like Map, Set, and WeakMap are available in modern stable browser releases, not behind flags. For more browser compat info check out kangax's es6-compat-table.

@akre54
Copy link
Collaborator Author

akre54 commented Sep 23, 2014

Yep, that's what I was basing my opinion on. Chrome37 has WeakSet and WeakMap but no Set and no Map. The constructors and their arguments are still getting worked out last I remember.

@jdalton
Copy link
Contributor

jdalton commented Sep 23, 2014

Chrome37 has WeakSet and WeakMap but no Set and no Map

Naw, it has Set, Map, and WeakMap.

The constructors and their arguments are still getting worked out last I remember.

Some extra API sugar around constructors is still being implemented across-engines but the set of functionality used by @megawac's proposal and that of #1862 are supported across modern stable browsers.

@megawac
Copy link
Collaborator

megawac commented Sep 23, 2014

Ya, it'd be even nicer to be able to do Array.from(Set(array)) for the no iterator case.

jsperf for your pleasure of current vs patch http://jsperf.com/set-uniq-underscore

@jdalton
Copy link
Contributor

jdalton commented Sep 23, 2014

Keep in mind if you go the Set route you'd have to modify _.uniq to match NaN too.
There's also the cost of Set on smaller arrays to consider.

@akre54
Copy link
Collaborator Author

akre54 commented Sep 23, 2014

OK then lets bikeshed this a bit more. There's some interesting changes this could open up in Underscore.

Naw, it has Set, Map, and WeakMap.

screen shot 2014-09-23 at 1 55 41 pm

@jdalton
Copy link
Contributor

jdalton commented Sep 23, 2014

Hmm, I must have enabled the experimental flag several versions ago and forgotten about it 😀
It's unflagged in Chrome 38 (next release) and supported in at least IE 11, Firefox 24-32, & Safari 6.2, 7.1, & 8.

Besides enabling use of Set, Map, and friends, detecting NaN in methods that traditionally performed === matches also aligns with ES7 Array#contains (moz nightly & v8 patches). It's something to keep in mind for 2.0.

@megawac megawac added this to the v2.0 milestone Sep 23, 2014
@jdalton
Copy link
Contributor

jdalton commented Oct 7, 2014

Annnd Chrome 38 stable is out with support for Set and Map enabled without flags.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants