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

Use ES6 Proxy to throw on non-existing assertions #407

Closed
mgol opened this Issue Mar 23, 2015 · 19 comments

Comments

Projects
None yet
6 participants
@mgol
Copy link

mgol commented Mar 23, 2015

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

This comment has been minimized.

Copy link
Member

keithamus commented Mar 23, 2015

Firefox 36 has Proxies with get support in. I've done a quick mockup of what we'd need to do (http://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

This comment has been minimized.

Copy link

mgol commented Mar 23, 2015

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

This comment has been minimized.

Copy link
Member

keithamus commented Mar 24, 2015

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

@mgol

This comment has been minimized.

Copy link

mgol commented Mar 24, 2015

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

Michał Gołębiowski

@keithamus

This comment has been minimized.

Copy link
Member

keithamus commented Mar 24, 2015

Yup 😄

@holyjak

This comment has been minimized.

Copy link

holyjak commented Apr 22, 2015

This will be nice!

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

@keithamus

This comment has been minimized.

Copy link
Member

keithamus commented Apr 22, 2015

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

This comment has been minimized.

Copy link

mgol commented Apr 22, 2015

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

This comment has been minimized.

Copy link

holyjak commented Apr 22, 2015

Thanks for the info!

@mgol

This comment has been minimized.

Copy link

mgol commented Jun 30, 2015

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

This comment has been minimized.

Copy link
Member

keithamus commented Jun 30, 2015

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

This comment has been minimized.

Copy link

mgol commented Jun 30, 2015

@mgol

This comment has been minimized.

Copy link

mgol commented Jul 1, 2015

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://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

This comment has been minimized.

Copy link
Member

keithamus commented Jul 1, 2015

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

This comment has been minimized.

Copy link
Member

keithamus commented Jul 1, 2015

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

PR's welcome @mzgol @jakubholynet

@robertrossmann

This comment has been minimized.

Copy link

robertrossmann commented Feb 8, 2016

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

This comment has been minimized.

Copy link

mgol commented Mar 4, 2016

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

This comment has been minimized.

Copy link

aaronjensen commented Mar 31, 2016

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

This comment has been minimized.

Copy link
Contributor

meeber commented Jun 27, 2016

Added in #721

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