Chai: [ES6] should.equal fails for Symbols

[meeber]

Hallo :D

I ran into this small issue while testing Symbol equality using the should syntax:

let prince = Symbol("Love");

expect(prince).to.equal(prince);  // Passes
prince.should.equal(prince);  // AssertionError: expected {} to equal {}

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!

[keithamus]

Thanks for the issue @meeber!

Where do you propose this change should go? Which line? We should make sure everything expect supports should also supports, so this is a valid bug.

[meeber]

Looks like this can be resolved in...

https://github.com/chaijs/chai/blob/816d526a0ea460271c3725f79f4f4c178b0a4a8d/lib/chai/interface/should.js#L12-L17

...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);
    }

[keithamus]

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.

[meeber]

Love that approach. Object.prototype.valueOf will just end up returning this for most non-primitives anyway unless the user shadowed valueOf or used Object.create(null).

I guess the main question is: If a user does shadow an object's valueOf with a function, is there any potential for an assertion to behave differently than before since this.valueOf() would now be passed in instead of this?

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 https://github.com/chaijs/chai/blob/816d526a0ea460271c3725f79f4f4c178b0a4a8d/test/should.js#L309-L320

[keithamus]

I guess the main question is: If a user does shadow an object's valueOf with a function, is there any potential for an assertion to behave differently than before since this.valueOf() would now be passed in instead of this?

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 typeof this === 'symbol' - seeing as every browser that supports Symbols also supports typeof Symbol() === 'symbol'.

Regardless, probably good to add a pair of Symbol tests...

Absolutely.

I'll mark this as PR wanted - based on the following criteria:

• The PR just adds the extra check for Symbols in the if - as described above
• The PR has tests to back up the correct behaviour for should
• Those tests should be conditional, so the tests still pass in browsers which don't support Symbols.

[meeber]

Sounds good :D

I'll submit a PR after work today if it's still open then.

Edit: Working on this now.