Skip to content

Conversation

@keithamus
Copy link
Member

Symbol.toStringTag is an available read only property in Node 6. The tests overwrite the property
and as such were failing in Node 6.0. This fixes the tests so that they don't assign on Symbol
if the property already exists.

It also alters the tests to use Symbol.toStringTag proper, rather than the sham string - so that
when toStringTag actually exists, it is actually used rather than the sham; allowing tests to pass
in environments which support toStringTag for real.

@lucasfcosta @davelosert if you could please check this over and merge when happy 😄

Symbol.toStringTag is an available read only property in Node 6. The tests overwrite the property
and as such were failing in Node 6.0. This fixes the tests so that they don't assign on Symbol
if the property already exists.

It also alters the tests to use Symbol.toStringTag proper, rather than the sham string - so that
when toStringTag actually exists, it is actually used rather than the sham; allowing tests to pass
in environments which support toStringTag for real.
@coveralls
Copy link

coveralls commented Apr 27, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling c1bb90b on keithamus:fix-node-6 into 42cd787 on chaijs:master.

@lucasfcosta
Copy link
Member

lucasfcosta commented Apr 27, 2016

The whole logic looks correct to me, it really fixes what you've described.
However, I've tried to find more about __@@toStringTag__ and I couldn't find info on it, would you mind giving me any links to check on it? Just because I feel like I should learn about how this works exactly.

@keithamus
Copy link
Member Author

__@@toStringTag__ was just a hacky way to sham the behaviour of Symbol.toStringTag, it has no bearing in the real world.

@lucasfcosta
Copy link
Member

@keithamus ah, makes total sense, I should've read the whole code, sorry for that
Anyway, good job, Keith!
@davelosert can merge as soon as he wants to 😄

@davelosert
Copy link
Member

davelosert commented Apr 30, 2016

@lucasfcosta @keithamus : Sorry for the delayed response - there is lots on my desk right now.
This looks very good for me - thanks for the fix @keithamus.

The lgtm is still pending though - should I merge this manually anyways?

LGTM

@keithamus
Copy link
Member Author

I think because @lucasfcosta didn't actually say LGTM it is still pending 😉

@keithamus
Copy link
Member Author

And now its not, because I said LGTM 😆

@davelosert davelosert merged commit 4a206b5 into chaijs:master Apr 30, 2016
@davelosert
Copy link
Member

@keithamus : Perfect and thanks! 😄

@keithamus keithamus deleted the fix-node-6 branch April 30, 2016 09:18
@lucasfcosta
Copy link
Member

Oh sorry, I totally forgot about LGTM
Great work guys 👍

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.

4 participants