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

Replace "instanceof Array" in transformer with "[object Array]" comparison #3985

Merged
merged 1 commit into from
Jun 5, 2015

Conversation

ide
Copy link
Contributor

@ide ide commented May 21, 2015

Testing with '[object Array]' is Object::toString.call element allows arrays from another JS context to be properly handled. The specific use case here is to support jest, which sets up JS contexts using Node/io.js's "vm" module. This approach works in ES3 environments in contrast with ES5's Array.isArray.

…rison

Testing with `'[object Array]' is Object::toString.call element` allows arrays from another JS context to be properly handled. The specific use case here is to support jest, which sets up JS contexts using Node/io.js's "vm" module. This approach works in ES3 environments in contrast with ES5's `Array.isArray`.
@lydell
Copy link
Collaborator

lydell commented May 21, 2015

In #3984 you also changed browser.coffee, but you haven't here. Is that intentional?

@ide
Copy link
Contributor Author

ide commented May 21, 2015

Yeah, since I didn't have a need for it and I didn't expect browser.coffee to run in a Node vm context (which was having trouble with instanceof Array in the first place).

@michaelficarra
Copy link
Collaborator

LGTM.

@DmitrySoshnikov
Copy link

Is it a good to go for merge? There is a dependency where we wait for this PR.

michaelficarra added a commit that referenced this pull request Jun 5, 2015
Replace "instanceof Array" in transformer with "[object Array]" comparison
@michaelficarra michaelficarra merged commit 1f197fc into jashkenas:master Jun 5, 2015
@DmitrySoshnikov
Copy link

Thanks!

@ide ide deleted the array-check branch June 16, 2015 20:55
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