ClojureScript

The array? predicate appears to be incomplete

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Environment:
    Probably all?

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.

  1. cljs_644_v3.patch
    05/Mar/14 6:17 PM
    1 kB
    Travis Vachon
  2. cljs_644_v4.patch
    12/Mar/14 2:25 PM
    2 kB
    Travis Vachon
  3. cljs_664.patch
    05/Mar/14 5:57 PM
    1 kB
    Travis Vachon
  4. cljs_664.patch
    10/Dec/13 9:49 AM
    1 kB
    Travis Vachon
  5. isArray.patch
    09/Dec/13 4:52 PM
    1.0 kB
    Travis Vachon

Activity

Hide
David Nolen added a comment -

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.

Show
David Nolen added a comment - 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.
Hide
Travis Vachon added a comment -

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?

Show
Travis Vachon added a comment - 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?
Hide
Travis Vachon added a comment -

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

Show
Travis Vachon added a comment - 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...
Hide
Travis Vachon added a comment -

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.

Show
Travis Vachon added a comment - 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.
Hide
Travis Vachon added a comment -

added {{git am}}able patch

Show
Travis Vachon added a comment - added {{git am}}able patch
Hide
David Nolen added a comment -

This patch need to be rebased to master.

Show
David Nolen added a comment - This patch need to be rebased to master.
Hide
Travis Vachon added a comment - - edited

sure! uploaded a rebased patch

Show
Travis Vachon added a comment - - edited sure! uploaded a rebased patch
Hide
Travis Vachon added a comment -

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

Show
Travis Vachon added a comment - oh actually, I just noticed this uses an older style of function access that causes warnings now, will update
Hide
Travis Vachon added a comment -

uploaded patch with same functionality, less warnings

Show
Travis Vachon added a comment - uploaded patch with same functionality, less warnings
Hide
David Nolen added a comment -

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

Show
David Nolen added a comment - Why do we need to mutate compiler-env can't you just add the target purely on the next line? Otherwise patch looks OK.
Hide
Travis Vachon added a comment -

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?

Show
Travis Vachon added a comment - 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?
Hide
David Nolen added a comment - - edited

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

Show
David Nolen added a comment - - edited Actually I do see what you mean. Yes let's do the swap! inside.
Hide
Travis Vachon added a comment -

cool! added an updated patch

Show
Travis Vachon added a comment - cool! added an updated patch

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: