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
[ES6] should.equal fails for Symbols #669
Comments
Thanks for the issue @meeber! Where do you propose this change should go? Which line? We should make sure everything |
Looks like this can be resolved in... chai/lib/chai/interface/should.js Lines 12 to 17 in 816d526
...by changing... function shouldGetter() {
if (this instanceof String || this instanceof Number || this instanceof Boolean ) {
return new Assertion(this.valueOf(), null, shouldGetter);
}
return new Assertion(this, null, shouldGetter);
} ...to... function shouldGetter() {
if (this instanceof String || this instanceof Number || this instanceof Boolean || this instanceof Symbol) {
return new Assertion(this.valueOf(), null, shouldGetter);
}
return new Assertion(this, null, shouldGetter);
} Although that would throw a ReferenceError in pre-ES6 Node/browsers without Symbol support. And it's a really long line. So maybe: function shouldGetter() {
if (this instanceof String
|| this instanceof Number
|| this instanceof Boolean
|| 'function' === typeof Symbol && this instanceof Symbol) {
return new Assertion(this.valueOf(), null, shouldGetter);
}
return new Assertion(this, null, shouldGetter);
} |
I wonder if we should just ducktype that line instead? function shouldGetter() {
if (typeof this.valueOf === 'function') {
return new Assertion(this.valueOf(), null, shouldGetter);
}
return new Assertion(this, null, shouldGetter);
} Might be an interesting solution to explore. |
Love that approach. I guess the main question is: If a user does shadow an object's A stylistic alternative to this approach: function shouldGetter() {
var val = typeof this.valueOf === 'function' ? this.valueOf() : this;
return new Assertion(val, null, shouldGetter);
} Regardless, probably good to add a pair of Symbol tests to Lines 309 to 320 in 816d526
|
Yeah... you raise a good point here. This could be potentially dangerous for that exact reason actually. Probably best just to add the extra Symbol code there, as per your original comment. In fact we could handle it a bit easier by doing
Absolutely. I'll mark this as PR wanted - based on the following criteria:
|
Sounds good :D I'll submit a PR after work today if it's still open then. Edit: Working on this now. |
- Fix bug when testing Symbol equality with should syntax (chaijs#669) - Fix bug when testing Symbol keys in Map and Set - Add Symbol tests for assert, expect, and should
- Fix bug when testing Symbol equality with should syntax (chaijs#669) - Fix bug when testing Symbol keys in Map and Set - Add Symbol tests for assert, expect, and should
Hallo :D
I ran into this small issue while testing Symbol equality using the
should
syntax:Adding
|| this instanceof Symbol
to the shouldGetter resolved the issue. I'm not sure if a PR is desired since it's ES6-specific?Thanks!
The text was updated successfully, but these errors were encountered: