Skip to content
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

Getter for Should syntax now uses valueOf for primitives #379

Merged
merged 1 commit into from Feb 24, 2015

Conversation

@dcneiner
Copy link
Contributor

@dcneiner dcneiner commented Feb 24, 2015

While this PR doesn't fix a bug when Chai is used by itself, it does make it more compatible with babel and its core-js dependency (and possibly other ES6 transpilation tools).

The issue that led to this PR occurred when using the latest version of babel (and subsequently core-js). Our assertions involving Numbers would fail:

var test = 1;
test.should.equal( 1 );

This would throw an error AssertionError: expected {} to equal 1 when used with babel.

After the update to babel and this error, we checked shouldjs and it did not exhibit this error. The reason for this, is that shouldjs uses valueOf for primitives instead of calling back into their constructors:

core-js is overwriting the Number prototype and constructor to support octal and binary (https://github.com/zloirock/core-js/commits/master/src/es6.number.constructor.js)

I would be more than willing to attempt unit tests around how the existing implementation could break, but it is such an edge case and using valueOf is a common way to unwrap primitives. All tests (via npm test) in this repo passed with the change in place.

We just spent the weekend and last few days switching our projects over from shouldjs to Chai because of how awesome it is. We love this project and I am happy to change this request as needed or to provide more information.

This allows the should syntax to be more resilient when
dealing with modified primitive constructors which
may occur more frequently with ES6 to ES5 transpilation.
keithamus added a commit that referenced this pull request Feb 24, 2015
Getter for Should syntax now uses valueOf for primitives
@keithamus keithamus merged commit 2cad0dc into chaijs:master Feb 24, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@keithamus
Copy link
Member

@keithamus keithamus commented Feb 24, 2015

Very detailed report, thanks for explaining it so well @dcneiner. Glad to hear you're enjoying using Chai 😄

@dcneiner
Copy link
Contributor Author

@dcneiner dcneiner commented Feb 24, 2015

Awesome @keithamus, thanks!

@dcneiner dcneiner deleted the dcneiner:should-primitive-fix branch Feb 24, 2015
@keithamus keithamus mentioned this pull request Jul 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants