Don’t compare non-enumerable symbols

[lucaswerkmeister]

Since version 4.0.1 (67d704ca19ff9e1acd7ddea9fbf7a4157917921a / #81, CC @snewcomer and @keithamus), deep-eql compares non-enumerable symbols. I find this surprising and unintuitive, and it’s different from string keys, where only enumerable keys are compared:

> key = 'x';
> deepEql(Object.defineProperty({}, key, { value: 'x', enumerable: false }), Object.defineProperty({}, key, { value: 'y', enumerable: false }))
true
> key = Symbol.for('x');
> deepEql(Object.defineProperty({}, key, { value: 'x', enumerable: false }), Object.defineProperty({}, key, { value: 'y', enumerable: false }))
false

@meeber in https://github.com/chaijs/chai/issues/1054#issuecomment-331317911 also only suggested that enumerable symbols should be compared, and I suggest changing deep-eql to only compare these.

[lucaswerkmeister]

To provide some context for the “unintuitive” description, if you’re curious: in m3api-query, I use non-enumerable symbol properties to attach additional information to objects that users of my library might find useful, but won’t necessarily need all of the time; I don’t need them to show up in console.log(), for example. (In fact, I was under the mistaken impression that, without a reference to the symbol, you couldn’t access the property at all: I was unaware that Object.getOwnPropertyNames() and Object.getOwnPropertySymbols() include non-enumerable properties in their return values.) In tests that aren’t focused on these additional properties, I compare the objects returned by my library with object literals that only have the “main” properties, and expect those comparisons to succeed; after an npm update, they failed.

The assertion failure message was also not easy to understand, unfortunately:

      AssertionError: expected { Object (revid) } to deeply equal { revid: 123 }                                                                   
      + expected - actual                                                                                                                          

But that’s probably not deep-eql’s fault, I think the message formatting is somewhere else in Chai.

[keithamus]

Thanks for the issue. It’s definitely valid. We should only compare enumerable properties.

As an aside, the message formatting code is in Loupe. If you have an idea of what the error message should have looked like, it’d be amazing if you could file an issue and we can work to make it better.

[lucaswerkmeister]

Thanks! Regarding Loupe, not sure what should be done, especially if it’s a general-purpose “inspect” library… I played with it a few minutes and couldn’t reproduce the { Object (revid) } output, so I might be doing something wrong.

I guess with this change merged, it shouldn’t matter anyways ^^

[matthew-dean]

I was also surprised by this breaking change. I updated to the latest chai (4.3.7) and all of the sudden, non-enumerable properties are included in the equality comparison.