Repository: chaijs/chai
Pull request #721 (2016-06-08 21:52:39)

Throw when non-existent property is read
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:

```js
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](https://github.com/domenic/chai-as-promised/blob/30183a7de6c4092b1b44ad9b5197696465e64e23/lib/chai-as-promised.js#L92) from `chai-as-promised`: 

```js
return typeof assertion.then === "function" ? assertion : assertion._obj;
```

Instead, plugin authors would have to guard such checks like so:

```js
// 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 traces 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**](http://babeljs.io/docs/learn-es2015/#proxies).

> Unsupported feature
> Due to the limitations of ES5, Proxies cannot be transpiled or polyfilled.

I also found [this module on NPM](https://www.npmjs.com/package/babel-plugin-proxy) 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 work around 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](https://github.com/chaijs/chai/pull/721/files#diff-3e1a5ec5945443e44dfa543f4e800a13R20).

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¢

![](https://media1.giphy.com/media/sTUWqCKtxd01W/200.gif)
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 #723)
1. This PR doesn't play nicely with promises, but the fix might be as simple as white-listing `.then` when it comes to throwing errors for a non-existent property.
1. 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 #723.
- 94 tests are broken with this PR. I believe this is a combination of #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](https://github.com/mochajs/mocha/blob/e939d8e4379a622e28064ca3a75f3e1bda7e225b/lib/runnable.js#L327) 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.
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 with `.then` whitelisted 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.)

I'll write up my thoughts on this approach vs. removing property assertions.
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 domenic/chai-as-promised#157, then everything will work fine, right?
lucasfcosta: @keithamus Yes it will! I think [this post](https://github.com/domenic/chai-as-promised/pull/157#discussion_r67452379) pretty much explains what was going on. It was a simple problem which went unnoticed until @meeber did the great job of finding it out :smile: 

[This post](https://github.com/domenic/chai-as-promised/pull/157#issuecomment-226901950) 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.
lucasfcosta: @keithamus Yes it will! I think [this post](https://github.com/domenic/chai-as-promised/pull/157#discussion_r67452379) pretty much explains what was going on. It was a simple problem which went unnoticed until @meeber did the great job of finding it out :smile: 

[This post](https://github.com/domenic/chai-as-promised/pull/157#issuecomment-226901950) 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.
meeber: Just to be clear, https://github.com/domenic/chai-as-promised/pull/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...

Patch:
diff --git a/lib/chai/assertion.js b/lib/chai/assertion.js
index 99aef48..a5033d4 100644
--- a/lib/chai/assertion.js
+++ b/lib/chai/assertion.js
@@ -33,6 +33,8 @@ module.exports = function (_chai, util) {
     flag(this, 'ssfi', stack || Assertion);
     flag(this, 'object', obj);
     flag(this, 'message', msg);
+
+    return util.proxify(this);
   }
 
   Object.defineProperty(Assertion, 'includeStack', {
diff --git a/lib/chai/core/assertions.js b/lib/chai/core/assertions.js
index f3c66b0..388ade2 100644
--- a/lib/chai/core/assertions.js
+++ b/lib/chai/core/assertions.js
@@ -300,7 +300,7 @@ module.exports = function (chai, _) {
         true === flag(this, 'object')
       , 'expected #{this} to be true'
       , 'expected #{this} to be false'
-      , this.negate ? false : true
+      , flag(this, 'negate') ? false : true
     );
   });
 
@@ -322,7 +322,7 @@ module.exports = function (chai, _) {
         false === flag(this, 'object')
       , 'expected #{this} to be false'
       , 'expected #{this} to be true'
-      , this.negate ? true : false
+      , flag(this, 'negate') ? true : false
     );
   });
 
@@ -1528,7 +1528,7 @@ module.exports = function (chai, _) {
         result
       , 'expected #{this} to satisfy ' + _.objDisplay(matcher)
       , 'expected #{this} to not satisfy' + _.objDisplay(matcher)
-      , this.negate ? false : true
+      , flag(this, 'negate') ? false : true
       , result
     );
   }
diff --git a/lib/chai/utils/addChainableMethod.js b/lib/chai/utils/addChainableMethod.js
index b3b2e70..d2c3209 100644
--- a/lib/chai/utils/addChainableMethod.js
+++ b/lib/chai/utils/addChainableMethod.js
@@ -10,6 +10,7 @@
 
 var transferFlags = require('./transferFlags');
 var flag = require('./flag');
+var proxify = require('./proxify');
 
 /*!
  * Module variables
@@ -104,7 +105,7 @@ module.exports = function (ctx, name, method, chainingBehavior) {
         }
 
         transferFlags(this, assert);
-        return assert;
+        return proxify(assert);
       }
     , configurable: true
   });
diff --git a/lib/chai/utils/index.js b/lib/chai/utils/index.js
index 607a4a9..57ea5ed 100644
--- a/lib/chai/utils/index.js
+++ b/lib/chai/utils/index.js
@@ -152,3 +152,9 @@ exports.getOwnEnumerableProperties = require('./getOwnEnumerableProperties');
  */
 
 exports.checkError = require('check-error');
+
+/*!
+ * Proxify util
+ */
+
+exports.proxify = require('./proxify');
diff --git a/lib/chai/utils/proxify.js b/lib/chai/utils/proxify.js
new file mode 100644
index 0000000..581a9dc
--- /dev/null
+++ b/lib/chai/utils/proxify.js
@@ -0,0 +1,35 @@
+/*!
+ * Chai - proxify utility
+ * Copyright(c) 2012-2014 Jake Luer <jake@alogicalparadox.com>
+ * MIT Licensed
+ */
+
+/**
+ * # proxify(object)
+ *
+ * Return a proxy of given object that throws an error when a non-existent
+ * property is read. (If Proxy or Reflect is undefined, then return object
+ * without modification.)
+ *
+ * @param {Object} obj
+ * @namespace Utils
+ * @name proxify
+ */
+
+module.exports = function proxify (obj) {
+  if (typeof Proxy === 'undefined' || typeof Reflect === 'undefined')
+    return obj;
+
+  return new Proxy(obj, {
+    get: function getProperty (target, property) {
+      // Don't throw error on Symbol properties such as Symbol.toStringTag, nor
+      // on .then because it's necessary for promise type-checking.
+      if (typeof property === 'string' &&
+          property !== 'then' &&
+          !Reflect.has(target, property))
+        throw Error('Invalid Chai property: ' + property);
+
+      return target[property];
+    }
+  });
+};


Test patch:
diff --git a/test/expect.js b/test/expect.js
index d361120..5706b41 100644
--- a/test/expect.js
+++ b/test/expect.js
@@ -10,6 +10,31 @@ describe('expect', function () {
     expect('foo').to.equal('foo');
   });
 
+  it('invalid property', function () {
+    if (typeof Proxy === 'undefined' || typeof Reflect === 'undefined') return;
+
+    err(function () {
+      expect(42).pizza;
+    }, 'Invalid Chai property: pizza');
+
+    err(function () {
+      expect(42).to.pizza;
+    }, 'Invalid Chai property: pizza');
+
+    err(function () {
+      expect(42).to.be.a.pizza;
+    }, 'Invalid Chai property: pizza');
+
+    err(function () {
+      expect(42).to.equal(42).pizza;
+    }, 'Invalid Chai property: pizza');
+
+    // .then is excluded from property validation for promise support
+    expect(function () {
+      expect(42).then;
+    }).to.not.throw();
+  });
+
   it('no-op chains', function() {
     function test(chain) {
       // tests that chain exists
diff --git a/test/should.js b/test/should.js
index e577786..0aec0d6 100644
--- a/test/should.js
+++ b/test/should.js
@@ -7,6 +7,31 @@ describe('should', function() {
     should.not.equal('foo', 'bar');
   });
 
+  it('invalid property', function () {
+    if (typeof Proxy === 'undefined' || typeof Reflect === 'undefined') return;
+
+    err(function () {
+      (42).should.pizza;
+    }, 'Invalid Chai property: pizza');
+
+    err(function () {
+      (42).should.be.pizza;
+    }, 'Invalid Chai property: pizza');
+
+    err(function () {
+      (42).should.be.a.pizza;
+    }, 'Invalid Chai property: pizza');
+
+    err(function () {
+      (42).should.equal(42).pizza;
+    }, 'Invalid Chai property: pizza');
+
+    // .then is excluded from property validation for promise support
+    (function () {
+      (42).should.then;
+    }).should.not.throw();
+  });
+
   it('no-op chains', function() {
     function test(chain) {
       // tests that chain exists
diff --git a/test/utilities.js b/test/utilities.js
index ad2b35f..66d6a81 100644
--- a/test/utilities.js
+++ b/test/utilities.js
@@ -844,4 +844,39 @@ describe('utilities', function () {
       expect(gettem(obj)).to.have.same.members([cat, dog, bird]);
     });
   });
+
+  describe('proxified object', function () {
+    if (typeof Proxy === 'undefined' || typeof Reflect === 'undefined') return;
+
+    var proxify;
+
+    beforeEach(function () {
+      chai.use(function (_chai, _) {
+        proxify = _.proxify;
+      });
+    });
+
+    it('returns property value if an existing property is read', function () {
+      var pizza = proxify({mushrooms: 42});
+
+      expect(pizza.mushrooms).to.equal(42);
+    });
+
+    it('throws error if a non-existent property is read', function () {
+      var pizza = proxify({});
+
+      expect(function () {
+        pizza.mushrooms;
+      }).to.throw('Invalid Chai property: mushrooms');
+    });
+
+    // .then is excluded from property validation for promise support
+    it('doesn\'t throw error if non-existent `then` is read', function () {
+      var pizza = proxify({});
+
+      expect(function () {
+        pizza.then;
+      }).to.not.throw();
+    });
+  });
 });