Skip to content

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

Closed
ide wants to merge 1 commit intojashkenas:masterfrom
ide:array-check
Closed

Replace "instanceof Array" in transformer with "[object Array]" comparison#3984
ide wants to merge 1 commit intojashkenas:masterfrom
ide:array-check

Conversation

@ide
Copy link
Copy Markdown
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.

Array.isArray 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.

The downside to this patch is that IE8 needs a polyfill for Array.isArray. If IE8 support for the in-browser transform (not the output of the coffeescript compiler) is important, it would be simple to include a polyfill with coffee-script.
@michaelficarra
Copy link
Copy Markdown
Collaborator

Sorry, we can't accept PRs that drop ES3 support.

@michaelficarra
Copy link
Copy Markdown
Collaborator

Though we could do a similar check, "[object Array]" is {}.toString.call element

@ide
Copy link
Copy Markdown
Contributor Author

ide commented May 21, 2015

I was hoping some kind of shim would be acceptable for environments that don't support Array.isArray natively, like

isArray = (value) ->
  if Array.isArray?
    Array.isArray value
  else
    '[object Array]' is Object::toString.call element

@ide ide changed the title Use "Array.isArray" instead of "instanceof Array" Replace "instanceof Array" in transformer with "[object Array]" comparison May 21, 2015
@ide
Copy link
Copy Markdown
Contributor Author

ide commented May 21, 2015

@michaelficarra: I updated the commit with your suggestion of checking with toString. Could you take a look please?

edit: I didn't realize that github doesn't update closed PRs. New PR at #3985. Sorry about that.

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.

2 participants