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

Use native Array.isArray #1552

Closed
wants to merge 3 commits into from

Conversation

kt3k
Copy link
Contributor

@kt3k kt3k commented May 4, 2016

No description provided.

@jmm
Copy link
Collaborator

jmm commented May 5, 2016

Hello @kt3k, thanks for contributing. LGTM -- I'll merge, but before I do a couple things:

  • How about instead of repeating Array.isArray we do this:

    - var isarray = require('isarray');
    + var isArray = Array.isArray;

    Then of course change all isarray => isArray.

  • I see that you retained isarray in package.json and bumped the version. Did you try to determine whether there's anything else still using that dep? Maybe we can just get rid of it now.

Notes: according to my research v8 added Array.isArray in v1.3.16, which would make it available from Node v0.1.15.

@kt3k
Copy link
Contributor Author

kt3k commented May 5, 2016

var isArray = Array.isArray;

OK. That looks better. I'll change it.

I see that you retained isarray in package.json and bumped the version. Did you try to determine whether there's anything else still using that dep? Maybe we can just get rid of it now.

I keep isarray in devDependencies because it's used in test/array.js as an example of bundleing of npm modules. It seems purely used as an example of npm module, and I think it's ok to keep it (because it only affects tests).

@jmm
Copy link
Collaborator

jmm commented May 5, 2016

@kt3k Thanks!

I keep isarray in devDependencies because it's used in test/array.js as an example of bundleing of npm modules. It seems purely used as an example of npm module, and I think it's ok to keep it (because it only affects tests).

Oh ok, sorry, I hadn't noticed that you moved it to devDeps. Nonetheless, if that's the only thing it's used for I'm going to see if there's something else that we're depending on for some other reason that can be substituted so we can just eliminate isarray.

@kt3k
Copy link
Contributor Author

kt3k commented May 6, 2016

I replaced test cases which use 'isarray' by ones using 'xtend' instead, and removed 'isarray' from devDeps. Looks cleaner!

@jmm jmm mentioned this pull request May 6, 2016
@jmm
Copy link
Collaborator

jmm commented May 6, 2016

@kt3k Thanks! I had actually done that part myself locally yesterday but hadn't had the chance to push yet. I took a look at your latest commit and I certainly appreciate you doing that, but for that part I used what I committed yesterday only because it's a smaller diff. I replaced isarray with defined which ended up with the same assertion methods and number of assertions (and even expected value!).

I squashed your first 2 commits together and merged via #1555. Thanks again!

@jmm jmm closed this May 6, 2016
@kt3k
Copy link
Contributor Author

kt3k commented May 6, 2016

My pleasure! Thanks for merging 😄
Appreciate your quick response!

@jmm
Copy link
Collaborator

jmm commented May 6, 2016

Published in 13.0.1. Not to discourage you from contributing, but just FYI -- responses usually aren't so quick.

@kt3k kt3k deleted the feature/use-native-isarray branch May 9, 2016 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants