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

Add ES6 Symbol support #672

Merged
merged 1 commit into from
Apr 8, 2016
Merged

Add ES6 Symbol support #672

merged 1 commit into from
Apr 8, 2016

Conversation

meeber
Copy link
Contributor

@meeber meeber commented Apr 8, 2016

Changes:

Notes:

  • No symbol tests were added to the key-checking block because of a separate bug. Will open a new issue on it soon. The bug occurs because both the expected and actual array of keys were being sorted in the assertKeys function using Array.prototype.sort but this throws a TypeError when one of the keys is a Symbol in a Map or a Set.
  • No Symbol tests were added to the NaN block because JavaScript's isNaN() throws a TypeError when passed a Symbol. I don't know if Chai is supposed to mimic this behavior with its NaN test or instead return true?
  • No Symbol tests that intentionally cause an error (and then test the string) were added. This is because the currently published version of Mocha doesn't correctly display AssertionError messages containing Symbols. However this issue appears to be resolved in Mocha's master branch; at least it displays "{}" to represent a Symbol.
  • I was unable to use the more succinct typeof this === 'symbol' approach mentioned in [ES6] should.equal fails for Symbols #669 because this is wrapped in an object and thus returns 'object' as its typeof.

@meeber
Copy link
Contributor Author

meeber commented Apr 8, 2016

On second thought, this Map/Set fix might be problematic.

If a user already has a keys-related test written that uses Map or Set (without any Symbol keys), and if it relies on the parameters in the AssertionError message to be sorted via Array.prototype.sort, then I believe their test would stop passing.

It's gonna be a little awkward to respect the existing behavior of sorting the expect/actual in the AssertionError message for Map and Set key checks but not sort them if either contains keys that are Symbols.

@keithamus
Copy link
Member

Hey @meeber thanks for the PR!

In the nicest way possible; try not to do too much in one PR. If you dial this back to just the fix discussed in #669 and then we can work on how to address the keys issue separately - without holding up the fix to Symbol equality. Sound good?

@meeber
Copy link
Contributor Author

meeber commented Apr 8, 2016

Agree 100%. Was actually doing that when you posted.

The changes have been made, only #669 fix and tests are in now.

@keithamus
Copy link
Member

In that case, fantastic work @meeber 😄

@keithamus
Copy link
Member

LGTM. @lucasfcosta?

@meeber
Copy link
Contributor Author

meeber commented Apr 8, 2016

Gotta run to work now so I'll open issues for the other stuff later. Thanks!

@lucasfcosta
Copy link
Member

LGTM.
Nice work and thanks for pointing out other issues.

@lucasfcosta lucasfcosta merged commit 6b640f7 into chaijs:master Apr 8, 2016
@meeber meeber deleted the symbol-support branch August 6, 2017 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants