Un-deprecate `length` and add guard #897

Merged
merged 2 commits into from Jan 3, 2017

Conversation

Projects
None yet
4 participants
@meeber
Contributor

meeber commented Jan 2, 2017

This PR is broken into two commits. The first commit makes length and lengthOf true aliases, with documentation updated to remove the deprecation notice and favor lengthOf over length. The second commit adds a guard against chaining length directly off of an uninvoked method assertion.

refactor(lengthOf): favor it over `length`
- The method part of the `length` assertion was slated for deprecation
  due to a compatibility issue in legacy environments. The decision to
  deprecate `length` was reversed per #684. This commit replaces the
  deprecation notice with a recommendation to favor `lengthOf` over
  `length` due to the compatibility issue.

- The `lengthOf` assertion wasn't a true alias of `length` because it
  was a method assertion instead of a chainable method assertion. This
  commit changes `lengthOf` into a chainable method assertion that's
  identical to `length`, and updates tests and docs accordingly.

- Updates docs to classify `length` as an alias of `lengthOf`.

- Updates docs of related assertions to use `lengthOf` instead of
  `length`.
@shvaikalesh

This comment has been minimized.

Show comment
Hide comment
@shvaikalesh

shvaikalesh Jan 2, 2017

Member

Great work, @meeber.

However, I think trapping length makes things more complicated than they could be. What about checking configurable attr of a function's length and set up a getter?

EDIT: why that's better:

  • proxiyfy: SRP (well, more specific responsibility), -1 parameter, easier logic
  • Works down to Node.js 4
  • Will make proxy optimizable
Member

shvaikalesh commented Jan 2, 2017

Great work, @meeber.

However, I think trapping length makes things more complicated than they could be. What about checking configurable attr of a function's length and set up a getter?

EDIT: why that's better:

  • proxiyfy: SRP (well, more specific responsibility), -1 parameter, easier logic
  • Works down to Node.js 4
  • Will make proxy optimizable
@meeber

This comment has been minimized.

Show comment
Hide comment
@meeber

meeber Jan 2, 2017

Contributor

@shvaikalesh Seems like a good suggestion, even if only for the benefit of "Works down to Node.js 4". Can you elaborate on the third point about optimization?

Contributor

meeber commented Jan 2, 2017

@shvaikalesh Seems like a good suggestion, even if only for the benefit of "Works down to Node.js 4". Can you elaborate on the third point about optimization?

@lucasfcosta

This comment has been minimized.

Show comment
Hide comment
@lucasfcosta

lucasfcosta Jan 2, 2017

Member

Awesome work! Thanks @meeber!
As @shvaikalesh suggested, since we're targeting newer environments by fixing this problem with Proxies I think that checking for configurable length properties would be a great idea.

However, due to the fact that older browsers don't have neither configurable length nor Proxy support I think this is a problem we can't really handle without an explicit warning.

Member

lucasfcosta commented Jan 2, 2017

Awesome work! Thanks @meeber!
As @shvaikalesh suggested, since we're targeting newer environments by fixing this problem with Proxies I think that checking for configurable length properties would be a great idea.

However, due to the fact that older browsers don't have neither configurable length nor Proxy support I think this is a problem we can't really handle without an explicit warning.

@meeber

This comment has been minimized.

Show comment
Hide comment
@meeber

meeber Jan 2, 2017

Contributor

@lucasfcosta Just to make sure we're on the same page regarding your second point: With this PR, the proxy-based length protection would throw a helpful error message in environments that support proxies, but in other environments, it would still throw the somewhat vague error message that it currently does in master. Likewise, if I refactor this PR to instead use @shvaikalesh's suggestion, then the getter-based length protection would throw a helpful error message in environments that support configurable .length on functions, but in other environments, it would still throw the somewhat vague error message that it currently does in master. So either way, the error messages in the legacy environments aren't improved upon, but at least @shvaikalesh's idea allows more environments to benefit from the helpful error messages.

Contributor

meeber commented Jan 2, 2017

@lucasfcosta Just to make sure we're on the same page regarding your second point: With this PR, the proxy-based length protection would throw a helpful error message in environments that support proxies, but in other environments, it would still throw the somewhat vague error message that it currently does in master. Likewise, if I refactor this PR to instead use @shvaikalesh's suggestion, then the getter-based length protection would throw a helpful error message in environments that support configurable .length on functions, but in other environments, it would still throw the somewhat vague error message that it currently does in master. So either way, the error messages in the legacy environments aren't improved upon, but at least @shvaikalesh's idea allows more environments to benefit from the helpful error messages.

@shvaikalesh

This comment has been minimized.

Show comment
Hide comment
@shvaikalesh

shvaikalesh Jan 2, 2017

Member

@meeber

Right now, we wrap every Assertion instance/chainable in proxy. That is kinda expensive. But what more expensive is that every [[Get]] getting trapped and invariant-checked.

Now

Correct method

Proxy {expensive} --> Assertion instance --> Assertion.prototype [hit!] -!- Object.prototype -!- null

Misspelled method

Proxy {expensive}[error!] -!- Assertion instance -!- Assertion.prototype [hit!] -!- Object.prototype -!- null

Suggested

Correct method

Assertion instance --> Assertion.prototype [hit!] -!- Proxy {expensive} -!- Object.prototype -!- null

Misspelled method

Assertion instance --> Assertion.prototype --> Proxy {expensive}[error!] -!- Object.prototype -!- null

EDIT: oops, sent too early.

So, by putting proxies upper on the prototype chain, [[Get]] will hit them only when they would throw and they won't reduce performance otherwise (tested in V8).

Member

shvaikalesh commented Jan 2, 2017

@meeber

Right now, we wrap every Assertion instance/chainable in proxy. That is kinda expensive. But what more expensive is that every [[Get]] getting trapped and invariant-checked.

Now

Correct method

Proxy {expensive} --> Assertion instance --> Assertion.prototype [hit!] -!- Object.prototype -!- null

Misspelled method

Proxy {expensive}[error!] -!- Assertion instance -!- Assertion.prototype [hit!] -!- Object.prototype -!- null

Suggested

Correct method

Assertion instance --> Assertion.prototype [hit!] -!- Proxy {expensive} -!- Object.prototype -!- null

Misspelled method

Assertion instance --> Assertion.prototype --> Proxy {expensive}[error!] -!- Object.prototype -!- null

EDIT: oops, sent too early.

So, by putting proxies upper on the prototype chain, [[Get]] will hit them only when they would throw and they won't reduce performance otherwise (tested in V8).

@meeber

This comment has been minimized.

Show comment
Hide comment
@meeber

meeber Jan 2, 2017

Contributor

@shvaikalesh Gotcha. Seems like another strong reason to limit proxy protection to undefined properties.

Contributor

meeber commented Jan 2, 2017

@shvaikalesh Gotcha. Seems like another strong reason to limit proxy protection to undefined properties.

@meeber

This comment has been minimized.

Show comment
Hide comment
@meeber

meeber Jan 3, 2017

Contributor

Pushed a new version of this that uses getters instead of proxies.

Contributor

meeber commented Jan 3, 2017

Pushed a new version of this that uses getters instead of proxies.

lib/chai/utils/addLengthGuard.js
+var config = require('../config');
+
+var fnDesc = Object.getOwnPropertyDescriptor(function () {}, 'length');
+var isFnLengthConfigurable = typeof fnDesc === 'object'

This comment has been minimized.

@shvaikalesh

shvaikalesh Jan 3, 2017

Member

Descriptor is guaranteed to be an object here.

@shvaikalesh

shvaikalesh Jan 3, 2017

Member

Descriptor is guaranteed to be an object here.

lib/chai/utils/addLengthGuard.js
+ */
+
+module.exports = function addLengthGuard (fn, assertionName, isChainable) {
+ if (isFnLengthConfigurable !== true) return fn;

This comment has been minimized.

@shvaikalesh

shvaikalesh Jan 3, 2017

Member

Maybe we can do if (!fnDesc.configurable) here?

@shvaikalesh

shvaikalesh Jan 3, 2017

Member

Maybe we can do if (!fnDesc.configurable) here?

lib/chai/utils/addLengthGuard.js
+
+ Object.defineProperty(fn, 'length', {
+ get: function () {
+ if (isChainable === true) {

This comment has been minimized.

@shvaikalesh

shvaikalesh Jan 3, 2017

Member

Maybe if (isChainable) will suffice?

@shvaikalesh

shvaikalesh Jan 3, 2017

Member

Maybe if (isChainable) will suffice?

@meeber

This comment has been minimized.

Show comment
Hide comment
@meeber

meeber Jan 3, 2017

Contributor

Updated!

Contributor

meeber commented Jan 3, 2017

Updated!

test/utilities.js
+
+ describe('addLengthGuard', function () {
+ var fnDesc = Object.getOwnPropertyDescriptor(function () {}, 'length');
+ if (typeof fnDesc !== 'object' || fnDesc.configurable !== true) return;

This comment has been minimized.

@shvaikalesh

shvaikalesh Jan 3, 2017

Member

I've missed this before. I believe, if (!fnDesc.configurable) would be enough.

@shvaikalesh

shvaikalesh Jan 3, 2017

Member

I've missed this before. I believe, if (!fnDesc.configurable) would be enough.

feat(utils): add length guard to methods
When the `length` assertion is chained directly off of an uninvoked
method, it references `function`'s built-in `length` property instead
of Chai's `length` assertion. This commit adds a guard to Chai methods
to detect this problem and throw a helpful error message that advises
the user on how to correct it.
@meeber

This comment has been minimized.

Show comment
Hide comment
@meeber

meeber Jan 3, 2017

Contributor

Gottem.

Contributor

meeber commented Jan 3, 2017

Gottem.

@shvaikalesh

This comment has been minimized.

Show comment
Hide comment
@shvaikalesh

shvaikalesh Jan 3, 2017

Member

Thanks for addressing the issues, @meeber! LGTM.

PS. Maybe we should change PR title to reflect the changes?

Member

shvaikalesh commented Jan 3, 2017

Thanks for addressing the issues, @meeber! LGTM.

PS. Maybe we should change PR title to reflect the changes?

@meeber meeber changed the title from Un-deprecate `length` and add proxy protection to Un-deprecate `length` and add guard Jan 3, 2017

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Jan 3, 2017

Member

This LGTM too. I'll force a merge, LGTM.co is no longer working so we need to disable and find a workable alternative.

Member

keithamus commented Jan 3, 2017

This LGTM too. I'll force a merge, LGTM.co is no longer working so we need to disable and find a workable alternative.

@keithamus keithamus merged commit c9bebe4 into chaijs:master Jan 3, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@shvaikalesh shvaikalesh referenced this pull request in chaijs/chaijs.github.io Jan 3, 2017

Merged

`length` as assertion => `lengthOf` #143

@meeber meeber deleted the meeber:proxy-length branch Aug 6, 2017

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