<< Back to previous view

[CLJS-664] The array? predicate appears to be incomplete Created: 05/Nov/13  Updated: 12/Mar/14  Resolved: 12/Mar/14

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Rasmus Buchmann Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

Probably all?


Attachments: Text File cljs_644_v3.patch     Text File cljs_644_v4.patch     Text File cljs_664.patch     Text File cljs_664.patch     Text File isArray.patch    

 Description   

The following command with fs the filesystem module from node still returns a js array:
js->clj (.readdirSync fs ".")))
A short irc discussion with dnolen revealed that the array? predicate is probably to blame. instanceof Array returns false for the result, while result.constructor == Array.



 Comments   
Comment by David Nolen [ 05/Nov/13 7:19 PM ]

This is likely because we're getting an Array from a different context. Node.js has Array.isArray and we could modify array? to emit this if the compile target is :nodejs.

Comment by Travis Vachon [ 09/Dec/13 1:44 PM ]

It looks like Array.isArray is available in some browsers as well, would it make sense to change the implementation to use that when possible and then fall back to goog.isArray?

Comment by Travis Vachon [ 09/Dec/13 1:46 PM ]

also, I found this, which seems helpful/relevant:

http://web.mit.edu/jwalden/www/isArray.html

this would probably mean that some behavior in older browsers would be different from newer browsers, but that seems unavoidable given the state of array detection in environments without Array.isArray...

Comment by Travis Vachon [ 09/Dec/13 4:52 PM ]

Use Array.isArray when building for node.js

This seemed to be the easiest way to do this, but I'm open to other ideas.

Comment by Travis Vachon [ 10/Dec/13 9:49 AM ]

added {{git am}}able patch

Comment by David Nolen [ 20/Jan/14 2:00 PM ]

This patch need to be rebased to master.

Comment by Travis Vachon [ 05/Mar/14 5:57 PM ]

sure! uploaded a rebased patch

Comment by Travis Vachon [ 05/Mar/14 6:10 PM ]

oh actually, I just noticed this uses an older style of function access that causes warnings now, will update

Comment by Travis Vachon [ 05/Mar/14 6:17 PM ]

uploaded patch with same functionality, less warnings

Comment by David Nolen [ 12/Mar/14 8:34 AM ]

Why do we need to mutate compiler-env can't you just add the target purely on the next line? Otherwise patch looks OK.

Comment by Travis Vachon [ 12/Mar/14 2:06 PM ]

Hm I don't think we can do it purely on that next line because we're still handing around the atom, but I do think I can just make it part of the mutation we're already doing below. Does that sound right?

Comment by David Nolen [ 12/Mar/14 2:14 PM ]

Actually I do see what you mean. Yes let's do the swap! inside.

Comment by Travis Vachon [ 12/Mar/14 2:26 PM ]

cool! added an updated patch

Comment by David Nolen [ 12/Mar/14 2:51 PM ]

fixed https://github.com/clojure/clojurescript/commit/5bc4f957ad98a6d4cd2de0576e5221e9b33bcdaf

Generated at Fri Aug 29 15:23:02 CDT 2014 using JIRA 4.4#649-r158309.