Throw when non-existent property is read

[meeber]

Hey guys, I was just playing around with ES6 Proxies to see what it'd look like if assertion chains threw an error if any of the properties were invalid.

For example, this assertion would throw an error:

expect(true).to.be.ture; // Oops, typo, throw Error

It seems pretty cool and doesn't break any of Chai's tests, but it'd be a breaking change for some plugins.

For example, you'd no longer be able to test for the existence of properties on an assertion like this code from chai-as-promised:

return typeof assertion.then === "function" ? assertion : assertion._obj;

Instead, plugin authors would have to guard such checks like so:

// ES5
return "then" in assertion && typeof assertion.then === "function"
     ? assertion
     : assertion._obj;

// ES6
return Reflect.has(assertion, "then") && typeof assertion.then === "function"
     ? assertion
     : assertion._obj;

Therefore, before rolling out a change like this, we'd want to work with plugin authors to get their code updated.

Also, there's an extra frame in the stack traces for assertion errors involving property assertions (but not for method assertions). Haven't found a way to get rid of it without also getting rid of the meaningful frame. A heavy-handed approach would be to filter out the Proxy frame manually from the stack string.

Anyway, this is a problem we should look to tackle soon. I'd argue that Chai's greatest weakness is the fact that invalid assertions silently pass. Hope this proof-of-concept sparks some ideas!

Edit: Whoops just saw #407, but it looks similar to the approach taken in this PR! :D

[lucasfcosta]

Great idea! Awesome work @meeber.
This really makes a lot of sense but I'm not feeling really comfortable about this now. The problem is not the code itself, the whole implementation looks good and the extra frame in the stack trace isn't a big problem, I'm just concerned about maintaining cross-browser compatibility.

I'd love to use ES6 on our codebase. It would be great to include Babel as a build step and make it possible to use ES6 without breaking browsers compatible with ES5 only, however even if we used Babel there would be no way of including this without breaking older browsers.

Unsupported feature
Due to the limitations of ES5, Proxies cannot be transpiled or polyfilled.

I also found this module on NPM but it doesn't seem to be very popular and there's also this warning on the README:

The problem is that proper proxy implementation requires native browser support (currently it works in Firefox and Edge). This plugin is proof of concept that proxies can be implemented with ES5 features. It is not suitable for production environments because performance impact is huge.

Unfortunately I don't think there's any other possibility of intercepting calls to every single property on an object and throw an error when it does not exist. I've done some research on this about two months ago due to a new project I was working into and I couldn't find any equivalent workaround for this, at that time I even had to run Node with an especial flag to enable Proxy support.

[meeber]

@lucasfcosta It's worth noting though that it only does the proxy stuff if Proxy is implemented, otherwise it falls back to the current ES5 behavior.

[lucasfcosta]

@meeber Thanks for pointing this out.
I should've paid more attention to this line.

Well, then I think we wouldn't have problems bringing those changes to the core. We'd just need to contact plugin developers and ask for their input on this.

Also, regarding the stack trace problem, I'm not a big fan of manipulating them as Strings. It feels too error-prone to me. I'll make sure to clone this and see if there's anything I can do about this.

[keithamus]

Great work @meeber, like the implementation and the tests.

Thanks for also doing the due diligence to see how this impacts our plugins. I think we could spend some time to reach out to plugin authors, perhaps raise some PRs to fix these issues, before merging this. Perhaps we'll have to earmark this PR for 5.0.0?

My 2¢

[meeber]

After work I plan to test out some other plugins with this to get a sense of how much this breaks. It's possible chai-as-promised was a special case with the .then. Normally flags are used which shouldn't break.

[meeber]

@lucasfcosta @keithamus I cloned a bunch of popular chai plugins and then ran their test suites twice, first using the current version of Chai via the master repo, and then using this PR's branch.

The main takeaways are:

1. Unrelated to this PR, we need to figure some stuff out related to chai-as-promised that broke due to recent changes in Chai (see chai-as-promised broken by change to addMethod and addProperty #723)
2. This PR doesn't play nicely with promises, but the fix might be as simple as black-listing .then when it comes to throwing errors for a non-existent property.
3. As far as I can tell, this PR doesn't cause any other major damage. There are a few minor issues, but at first glance they appear to be problems in the plugin tests themselves and not Chai. However, we'll need to resolve the first two issues above to be sure this PR doesn't cause any more problems in chai-as-promised and sinon-chai.

Findings

chai-as-promised

• 73 tests are broken with the current version of Chai. Details in chai-as-promised broken by change to addMethod and addProperty #723.
• 94 tests are broken with this PR. I believe this is a combination of chai-as-promised broken by change to addMethod and addProperty #723 and checking if .then exists on the assertion using typeof, which will throw an error if it doesn't exist since it's reading a non-existent property.

sinon-chai

• 0 tests are broken with the current version of Chai.
• Every single test breaks with this PR. I believe this is another issue promise-related issue, although indirect. I think each sinon-chai test returns the result of its assertion, which causes Mocha to check if the returned value is a Promise so it can handle asynchronicity. And how does Mocha check if the returned value is a promise? By checking if .then exists using typeof of course! Once again, this will throw an error if .then doesn't exist, since it's reading a non-existent property.

chai-jquery

• The same 4 tests are broken with both the current version of Chai and this PR. Haven't investigated why but seems to be related to property checking.

dirty-chai

• The same 2 tests are broken with both the current version of Chai and this PR. Again, not sure why. Seems related to chainable methods.

chai-webdriver

• 0 tests are broken with the latest version of Chai.
• 2 tests are broken with this PR. The tests themselves appear to be doing something weird by running one expect assertion, then taking the result of that, and running a second expect assertion on the first assertion in order to check if the .then property is defined. The process of checking for an undefined property on an assertion throws an error since it's reading a non-existent property.

chai-http

• 0 tests are broken with both the latest version of Chai and this PR.

chai-immutable

• 0 tests are broken with both the latest version of Chai and this PR.

chai-things

• The same 1 test is broken with both the latest version of Chai and this PR. Something about include.any not being a function. Haven't investigated further.

chai-spies

• 0 tests are broken with the latest version of Chai.
• 4 tests are broken with this PR. When constructing failed assertion error messages, it sometimes references a property called _his on the assertion, which doesn't exist and causes the proxy stuff to throw. Couldn't determine what the deal is with that from a quick scan. Thinking might be problem with the plugin code itself.

[keithamus]

Great work @meeber ❤️. I wonder if, considering this has such a large impact on the ecosystem, we should instead just park the whole proxy thing and focus our efforts in other areas? If we are going to have such an impact by trying to solve the whole property assertion thing, we could just rip the bandaid off and remove property assertions altogether. If either solution will take as much effort then the latter is probably preferred by the community at large.

To expand - we get lots of vocal complaints about property assertions (e.g. misspelling, linting rules disliking it, people unhappy with the design decision), and not many voice opinions of being in favour of it. Rather than keep trying to solve the symptoms of the issue, we could probably make the majority happy by removing property assertions altogether.

[meeber]

@keithamus I think it's worth having that conversation even if the impact of this proxy thing can be lowered to acceptable levels. Speaking of which, I just updated the PR to exclude .then from property validation, and I suspect it solves almost all of the issues, but can't test right now, gotta head to work :D

[meeber]

Success! Excluding .then from property validation fixed all proxy-specific problems in all plugins listed above except for one in chai-spies which I believe is an error in the plugin itself. (Note: until #723 is resolved, I won't be able to say for sure that it doesn't break anything in chai-as-promised.)

Edit: Doesn't break chai-as-promised either. :D

[meeber]

Feeling pretty good about this. Rebased and removed the WIP tag. I think it's ready to go.

[keithamus]

How do we feel about merging this one, given all of the info we have now? If we merge this in to get it into 4.0.0, and hope to get chaijs/chai-as-promised#157, then everything will work fine, right?

[lucasfcosta]

@keithamus Yes it will! I think this post pretty much explains what was going on with the chaining issue. It was a simple problem which went unnoticed until @meeber did the great job of finding it out 😄

This post also explains the whole fix. I'm feeling pretty confident with it since all we needed was to get into a simple if conditional block. Anyway, I'd also like to hear @meeber's opinion since the one who knows the most about this, I just know about domenic/chai-as-promised#157, this PR itself is what needs more attention.

[meeber]

Just to be clear, chaijs/chai-as-promised#157 fixes a problem that's unrelated to this PR. I just wanted to fix that issue first so I could verify that this PR doesn't break chai-as-promised. Which it doesn't! :D

This should be good to go once I rebase...

[meeber]

@keithamus @lucasfcosta Rebased and fixed the conflict (was minor). This should be good to go.

[keithamus]

Well, LGTM 😄

[lucasfcosta]

@meeber makes sense, I have mixed up things since we found out that bug due to this issue.

Disclaimer for anyone reading this in the future:
Losing the context of a promise is different from trying to access the then property on the assertion and then having an error thrown.

This LGTM too, nice work here, detailing why .then is a special case. Tests also look good.