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

isArray breaks #54

Closed
gregwebs opened this Issue Sep 5, 2012 · 9 comments

Comments

Projects
None yet
2 participants
@gregwebs

gregwebs commented Sep 5, 2012

The constructor of a proxied array does in fact === Array
However, everyone uses toString.apply to determine if an object is an array, and this changes once an object is proxied. I was just looking at the direct proxies ticket and it seems this issue is solved there. Is it possible for us to have a work around until then?

@disnet

This comment has been minimized.

Owner

disnet commented Sep 5, 2012

Ha! Yeah, I just ran into this bug too. It causes [].concat(proxiedArray) to give a array of length 1 instead of merging the proxiedArray's elements.

I'm not sure about a workaround yet. Maybe the direct proxy shim fixes the issue? I'll try hacking that into contracts.js and see if that fixes things.

@gregwebs

This comment has been minimized.

gregwebs commented Sep 5, 2012

was reading this: http://perfectionkills.com/instanceof-considered-harmful-or-how-to-write-a-robust-isarray/
apparently there are issues with checking the constructor in an iframe.
Is it possible for contracts.js to override the toString implementation?

@disnet

This comment has been minimized.

Owner

disnet commented Sep 5, 2012

Ugh, and that's a no on the direct proxy shim workaround.

% cat array.js 
require('harmony-reflect');

var arr = Proxy([1,2], {});
console.log([].concat([1,2]))
console.log([].concat(arr))
% node --harmony array.js
[ 1, 2 ]
[ { '0': 1, '1': 2 } ]
@disnet

This comment has been minimized.

Owner

disnet commented Sep 5, 2012

Right so the normal way of checking for an array without using the new Array.isArray builtin is:

Object.prototype.toString(arr) === '[object Array]'

So it would be straightforward to just patch Object.prototype.toString. The problem is that the functions we really care about like [].concat don't use toString, rather they consult the internal (c++) representation to decide if it's an array or not. Dito Array.isArray. So no amount of patching will help us there.

Humm...although we could just patch concat and Array.isArray directly to do the right thing by reimplementing their functionality in pure js for proxied arrays (and toString for people using the isArray polyfil).

@gregwebs

This comment has been minimized.

gregwebs commented Sep 6, 2012

yikes! We already don't care about performance of contracts, so re-implemenation in js is fine if it isn't buggy.

Is it possible for the contracts proxy behavior be modified? So that it does less for arrays and the proxy is added/removed earlier? I don't know if that actually make sense...

@disnet

This comment has been minimized.

Owner

disnet commented Sep 6, 2012

Yeah, I think that's probably the best option for now. Just do simple one-time checks when the contract get applied to a value (is the value actually an array, etc.) and don't wrap it. This means we don't get to check array accesses but things actually work.

disnet added a commit that referenced this issue Sep 6, 2012

@disnet

This comment has been minimized.

Owner

disnet commented Sep 6, 2012

Ok, contracts are no longer wrapping arrays. New version pushed out to npm too.

Hopefully direct proxies land soon.

@gregwebs

This comment has been minimized.

gregwebs commented Sep 6, 2012

awesome! I had to turn my contracts off :(
will test soon

@gregwebs

This comment has been minimized.

gregwebs commented Sep 9, 2012

seems to be working. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment