jonschlinkert / kind-of

Get the native JavaScript type of a value, fast. Used by superstruct, micromatch and many others!
https://github.com/jonschlnkert
MIT License
350 stars 38 forks source link

Create fallback for Array.isArray if used as a browser package #2

Closed dtothefp closed 9 years ago

dtothefp commented 9 years ago

I'd like to use this module to optionally replace some Lodash type functions in an Optimizely opensource Flux library. Seems like there should be a fallback for Array.isArray. You probably have a "slicker" way to do this, but if you're open to it, would be great to use this module.

tunnckoCore commented 9 years ago

Good point :+1: but wait @jonschlinkert merge #1 then do it again

tunnckoCore commented 9 years ago

@dtothefp @jonschlinkert or if you dont have problem with history and rights i can add it in my PR

dtothefp commented 9 years ago

@tunnckoCore I'm fine with this being moved into your PR. Maybe check with @jonschlinkert if this is a good way to do it from a performance perspective.

tunnckoCore commented 9 years ago

@dtothefp no it not hit performance.

@jonschlinkert btw offtopic, whats your thoughts, guys, for this https://github.com/tunnckoCore/mukla/issues/4? :) For me it looks awesome! :heart: lol

Actually mukla would be (only async) test runner - assert and style agnostic. In the issue I show whats the diffs and you'll can use other assertion library like expect.js, should or even builtin assert instead of assume

jonschlinkert commented 9 years ago

awesome, yeah that sounds great! thanks!

jonschlinkert commented 9 years ago

a good way to do it from a performance perspective.

Anything we implement as a fallback will be drastically slower than Array.isArray(), but since it's needed then it is what it is. we'll just let with benchmarks tell us which implementation to use.

(fwiw everyone likes to that benchmarks don't mean anything in real projects - IMHO that only makes sense if you're benchmarking something with a lot of moving parts, otherwise I don't where that nonsense came from).

tunnckoCore commented 9 years ago

Anything we implement as a fallback will be drastically slower

Actually not needed. We have this fallback in last line with toString.call-ing. So it will do 2-3 more checks to the end, so.. no big hit, imho.

awesome, yeah that sounds great! thanks!

for mukla or? lol :)

jonschlinkert commented 9 years ago

We have this fallback in last line with toString.call...

good point

for mukla

no, it looks like a good lib, but I'd like to stick with assert and should for now.

tunnckoCore commented 9 years ago

but I'd like to stick with assert and should for now.

I just want thoughts for the style and vision, not replace it in your flow :) I just got kind-of tests for examples/fixtures, because they are simple.

jonschlinkert commented 9 years ago

, not replace it in your flow :) I just got kind-of tests for examples/fixtures, because they are simple.

oh ok, no prob. I'll look it over and give some comments.

dtothefp commented 9 years ago

not sure if I'm following this conversation completely but if we don't do the conditional Array.isArray && Array.isArray(val) and just call Array.isArray in a non-supported environment it will most likely throw an error and halt any further execution. So, it seems redundant to check for an array twice, would it be better to remove Array.isArray and just use toString fallback?

tunnckoCore commented 9 years ago

@dtothefp review my last commit to my PR. It's enough to be

if (Array.isArray && Array.isArray(val)) {

if there's no support for .isArray method it won't call it and skip this check, so will go to the last line for fallback

tunnckoCore commented 9 years ago

@jonschlinkert @dtothefp just need merge :)

jonschlinkert commented 9 years ago

alright, thanks! I'll do it in a little bit

jonschlinkert commented 9 years ago

that looks good too, that's what I would have done, thx