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 Type-Checking #220

Closed
wants to merge 2 commits into from
Closed

Conversation

thomashuston
Copy link

In migrating an HTTP test suite that uses node-fetch from Mocha to Jest, a number of tests starting failing when trying to make assertions on cookies. When I dug into it, I traced it back to this Array type check in Headers.

Basically, because Jest executes tests in a vm, the class instances of core types like Array are different from those within Node's libraries like http, so instanceof checks against them fail.

The recommended approach for checking if an object is an array is to instead use the Array.isArray function.

This PR also includes a fallback for old versions of Node that don't include Array.isArray for maximum backwards compatibility.

@codecov-io
Copy link

codecov-io commented Jan 9, 2017

Current coverage is 100% (diff: 100%)

No coverage report found for master at d708acf.

Powered by Codecov. Last update d708acf...985b66b

@bitinn
Copy link
Collaborator

bitinn commented Jan 10, 2017 via email

@bitinn
Copy link
Collaborator

bitinn commented Jan 10, 2017

@TimothyGu I don't consider this a bug per se, do you want to fix it in v2 branch? It got babel for transpiling right?

@TimothyGu
Copy link
Collaborator

@bitinn, yeah, sure.

@thomashuston, is the Object.prototype.toString.call(obj) == '[object Array]' check necessary? In other words, is Array.isArray sufficient for it to work?

@thomashuston
Copy link
Author

@TimothyGu You're correct, we can just use Array.isArray. I just verified that it was present as far back as Node 0.10. Will update.

@TimothyGu
Copy link
Collaborator

Seems like this problem is already fixed in v2 branch, so PR closed. Thanks for the heads-up though.

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