Use ES6 Proxy to throw on non-existing assertions

[mgol]

With ES6 proxies it should be possible to have the following:

expect(someVariable).to.exiist;

throw which would remove one of the reasons why assertions on property access are problematic.

This will still need to work in browsers not supporting ES6 proxies (now: all browsers but this will soon change).

Refs #56.

[keithamus]

Firefox 36 has Proxies with get support in. I've done a quick mockup of what we'd need to do (https://jsbin.com/wawesicufe/3/edit). Essentially, we'd need to override the Assertion constructor to return a Proxy of itself, with the get trap set to throw if the property is undefined. So:

function Assertion(obj, msg, stack) {
    flag(this, 'ssfi', stack || arguments.callee);
    flag(this, 'object', obj);
    flag(this, 'message', msg);
    return new Proxy(this, {
        get: function(target, name) {
            if (name in target) {
                return target[name];
            }
            throw new Error('Cannot find property ' + name);
        }
    });
}

Of course, this is all well and good but until even a handful of engines support this - it's basically pointless to implement. Also we'd need to have a good test suite to back this up.

I'll mark this as hard-fix just because it's not happening any time soon, and the tests will be difficult to handle. I won't be encouraging a PR for this for now - due to lack of Proxy support.

[mgol]

Of course, this is all well and good but until even a handful of engines support this - it's basically pointless to implement.

IMO even doing it in 1-2 popular browsers is enough. People usually mostly test a lot manually on Firefox/Chrome during development and then run tests on everything supported via CI (Travis, Jenkins). So even having it just in Firefox & Chrome will make people see misspelled assertion names quickly and those tests won't get to other browsers in the broken state.

This won't cover assertions that are run only in specific browsers but that's the minority.

[keithamus]

Yeah, I agree. "handful of engines" was a figure of speech. We could probably implement this when Firefox/Chrome both get proxies.

[mgol]

That seems like a sensible strategy. :) Should we keep this bug open until
then?

Michał Gołębiowski

[keithamus]

Yup 😄

[holyjak]

This will be nice!

PS: Chrome/V8 supports it reportedly with the --harmony-proxies flag (and the same applies to iojs).

[keithamus]

A brief read of those issues seems to indicate existing support in Chrome for Proxies is buggy. That makes me nervous about supporting Proxies and getting an influx of difficult to track/fix bug reports. So I'd say we still hold off here.

[mgol]

Yes, current v8 Proxy implementation is broken, don't rely on that.

It will happen soon anyway, I'd just wait until the flag is removed.

Michał Gołębiowski

[holyjak]

Thanks for the info!

[mgol]

Edge has now even better Proxy support than Firefox, 21/21 points in the Kangax table. Edge officially comes out with Windows 10 in a month. So we have 2 browsers now, Firefox & Edge, maybe that's enough?

Chrome by default doesn't have the Proxy global so unless one runs it with unfinished flags it won't break.

[keithamus]

Thanks for the info @mzgol.

As mentioned, mostly concerned about users enabling the flag in chrome/node and having a broken implementation, to which we get issues because of the broken implementation. I think we'll pull the trigger as soon as Chrome/node either gets a working impl, or disables the broken one.

[mgol]

It's already disabled unless you specifically use the command line flag to enable the broken implementation. But OK, Chrome will most likely implement it within a few months anyway.

[mgol]

To be precise:

1. Node.js 0.10 doesn't have Proxy at all.
2. Node.js 0.12 doesn't have it by default, you'd need to explicitly pass the --harmony_proxies flag. Even just passing --harmony is not enough - the implementation is so broken it's excluded from the list even though Node.js 0.12 has an old enough v8 version that --harmony included a lot of experimental features (newer v8 versions enable just the "almost ready" ones).
3. io.js doesn't have Proxy and using --harmony or --es_staging won't enable it, only --harmony_proxies will.
4. Chrome doesn't have Proxy even if you enable "Experimental JavaScript features" on chrome:https://flags.

Users of Node.js/io.js versions that don't support proxies (currently all but that will change) will still get a broken implementation if they use the --harmony_proxies flag, now & in the future, since they're using old enough V8. Users that just use the default setup or even --harmony won't be affected.

So I don't really understand this remark:

mostly concerned about users enabling the flag in chrome/node and having a broken implementation

This will not change for existing versions. And I expect that even in a year there will be a lot of Node.js 0.12/older io.js users.

[keithamus]

We could look at feature detecting for broken Proxies. According to the spec (and firefox), new Proxy({}, {}); should work without throwing, but with node --harmony_proxies I get:

> new Proxy({}, {})
TypeError: object is not a function
    at repl:1:5
    at REPLServer.defaultEval (repl.js:132:27)
    at bound (domain.js:254:14)
    at REPLServer.runBound [as eval] (domain.js:267:12)
    at REPLServer.<anonymous> (repl.js:279:12)
    at REPLServer.emit (events.js:107:17)
    at REPLServer.Interface._onLine (readline.js:214:10)
    at REPLServer.Interface._line (readline.js:553:8)
    at REPLServer.Interface._ttyWrite (readline.js:830:14)
    at ReadStream.onkeypress (readline.js:109:10)
>

And in io.js I get:

> new Proxy({}, {})
TypeError: Proxy is not a function
    at repl:1:5
    at REPLServer.defaultEval (repl.js:155:27)
    at bound (domain.js:254:14)
    at REPLServer.runBound [as eval] (domain.js:267:12)
    at REPLServer.<anonymous> (repl.js:309:12)
    at emitOne (events.js:77:13)
    at REPLServer.emit (events.js:169:7)
    at REPLServer.Interface._onLine (readline.js:208:10)
    at REPLServer.Interface._line (readline.js:547:8)
    at REPLServer.Interface._ttyWrite (readline.js:824:14)
>

(As mentioned, Firefox passes this - as should a spec compliant implementation).

So in one foul swoop of feature detection, we could do:

var supportsProxies = false;
try {
  new Proxy({}, {});
  supportsProxies = true;
}  catch(e) {
}

[keithamus]

I'd say I think we're about ready to try supporting this.

PR's welcome @mzgol @jakubholynet

[robertrossmann]

The obvious solution would be to introduce a breaking change and require all assertions to be actual function calls:

expect({}).to.be.true()

Should.js had the same issue and they decided to get rid of assertions on property access in should.js 7 (changelog).

[mgol]

Update: latest Chrome (v49) now ships with Proxy support. This means Node.js 6, to be released in April, will support Proxy as well.

[aaronjensen]

The obvious solution would be to introduce a breaking change and require all assertions to be actual function calls

This is the obvious and right solution imo. It'd be a breaking change, sure, but it'd be worth it.

[meeber]

Added in #721