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

Proxy to Array fools most methods of checking whether Array is Array #13

Open
metamatt opened this issue Apr 15, 2013 · 11 comments
Open

Comments

@metamatt
Copy link
Contributor

I'm not quite sure how to motivate this problem without describing 3 different phenomena at once so please bear with me:

  1. JSON.stringify does not work on Proxy objects (it fails with "Illegal Access" exception).
  2. So I've been using Crockford's json2.js when I need to JSON-serialize Proxy objects. This works fine except if I have a Proxy to an Array (or to an object with an embedded Array), the generated JSON is just an object with numeric keys, not an Array (literally, more like { "0": 'foo', "1": 'bar' } instead of ['foo', 'bar'].
  3. The second thing is because the test json2.js uses to determine whether something is an array is the Object.prototype.toString.apply(candidate) === '[object Array]' test. Proxy to Array returns [object Object].

It's the third of these that I think is arguably a bug in the proxy implementation (or I'd like to know why not, and how to work around it!).

magi@ubuntu ~/src> node --harmony
> require('harmony-reflect')
> JSON2 = require('./json2')
> a = ['foo','bar']
[ 'foo', 'bar' ]
> p = Proxy(a, {})
{ '0': 'foo',
  '1': 'bar' }
> JSON.stringify(p)
illegal access
> JSON2.stringify(p)
'{"0":"foo","1":"bar"}'

Note there that the REPL's display of p shows it as an object with numeric keys, as I described in (2) above. So not only json2.js but also util.inspect (which the REPL uses) is treating p as a general Object, not as an Array.

Also note that the builtin JSON.stringify on a proxy just fails with "illegal access".

Here's the full battery of tests I can think of for whether something is an array (all of these return true for a, but mostly not for p):

> a instanceof Array
true
> Array.isArray(a)
true
> require('util').isArray(a)
true
> Object.prototype.toString.apply(a)
'[object Array]'
> p instanceof Array
true
> Array.isArray(p)
false
> require('util').isArray(p)
false
> Object.prototype.toString.apply(p)
'[object Object]'

So the instanceof test works on the proxy, but all the other tests fail. And a lot of code out there wants to use the Object.prototype.toString test. Now, the reasons for using the Object.prototype.toString test instead of the instanceof test might not be relevant to Node (namely, browser-side stuff with multiple frames or iframes). But is there any hope that Object.prototype.toString should be returning Array and not Object for arrays?

Some further details:

  • I was hoping that (1) would be fixed in more recent versions of Node, from https://chromiumcodereview.appspot.com/11312063 and https://code.google.com/p/v8/source/detail?r=12851.
  • (I don't know how to tell from Chromium or V8 bug reports what V8 version a fix goes into, but from the general timeframe of the fix, I was hoping this would be in V8 3.15.)
  • I just tried building Node.js from source, which currently comes with V8 3.16.17, and JSON.stringify(proxy) still fails as shown below.
@billmark
Copy link

I just checked these issues against the most recent version of v8 (3.17.16.2). Issue 1 from above (JSON.stringify dying on proxies) is fixed. Issue 3 from above (Arrays returning "object Object") is still there, as the following d8 console session shows:

V8 version 3.17.16.2 [console: readline]
d8> load("/home/parallels/git/experiments/node_modules/harmony-reflect/reflect.js");
undefined
d8> a = ["a","b","c"];
["a", "b", "c"]
d8> Object.prototype.toString.apply(a);
"[object Array]"
d8> var p = Proxy(a, {});
undefined
d8> print(p);
a,b,c
undefined
d8> print(a);
a,b,c
undefined
d8> Object.prototype.toString.apply(p);
"[object Object]"

@metamatt
Copy link
Contributor Author

Thanks for clarifying. I'd meant to report these results using that same Node version but had invoked the wrong one. Whoops.

I see the same thing as @billmark, which is nice because the JSON.stringify issue I complained about as (1) above actually is fixed, but now that it works, it turns out that it, just like JSON2.stringify and util.inspect, does not think that proxy-to-array is an array.

So scratch complaint (1), but (2) and (3) turn out to be even truer than I thought.

@billmark
Copy link

More details: I also checked against the slightly more recent "bleeding_edge" version of v8, "3.18.0 (candidate)", and it still has the issue that calling Object.prototype.toString on a proxied array returns "[object Object]" rather than "[object Array]".

tvcutsem pushed a commit that referenced this issue Apr 30, 2013
…s, so as to improve transparency for Arrays. Cf. issue #13
@tvcutsem
Copy link
Owner

I just committed a patch so that Array.isArray and Object.prototype.toString automatically unwrap proxies. Those methods now return the expected result on proxies for arrays:

var p = Proxy([] , {});
Object.prototype.toString.call(p) // [object Array]
Array.isArray(p) // true

However, JSON.stringify still serializes proxies-for-arrays as objects. This is because that algorithm internally uses its own isArray check, which my library cannot intercept. AFAICT, this is something that can be solved only by proxies directly provided by the engine. The alternative is to re-implement JSON.stringify in JavaScript itself, which doesn't seem to be a good alternative.

@billmark
Copy link

Thanks, Tom.

@coreybutler
Copy link

Has any of this made it's way to the npm module?

@tvcutsem
Copy link
Owner

Not yet. Thanks for bringing it to my attention. I'll upload a new npm package shortly.

@tvcutsem
Copy link
Owner

npm module v0.0.5 now contains the patches to Array.isArray and Object.prototype.toString to unwrap proxies.

@billmark
Copy link

Thanks, Tom!

@coreybutler
Copy link

Thanks!

@sieira
Copy link

sieira commented Nov 11, 2015

Regarding the JSON.stringify, a workaround is to do this:

  var proxy = new Proxy(obj, {
    get: function(target, property, receiver) {
      if (property === 'toJSON') {
        return () => target;
      }
 });

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

No branches or pull requests

5 participants