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

Prefer explicit string conversion #620

Merged
merged 3 commits into from Apr 18, 2016

Conversation

dashed
Copy link
Contributor

@dashed dashed commented Sep 10, 2015

ES6 Symbols will throw a runtime exception if implicitly coerced into a string.


Closes #619


Unsure if this is the right patch.

Do this for edge case such as: ES6 Symbols will throw a runtime exception if implicitly coerced into a string
@leebyron
Copy link
Collaborator

Thanks! Sorry for letting this sit - did you verify that this fixes the issue for you?

@dashed
Copy link
Contributor Author

dashed commented Dec 16, 2015

Yep verified. I probably should've put in a test for it.

@leebyron
Copy link
Collaborator

I'd like to test this. Could you include a test or use case that fails before this patch and works after?

@dashed
Copy link
Contributor Author

dashed commented Dec 20, 2015

@leebyron I wasn't quite sure how to test this such that:

Symbol() + ''; // this throws
String(Symbol()); // but this doesn't throw

I added a test case that I was working from; but didn't get far. I added a polyfill, core-js, which may be undesirable.


I think an alternative approach to testing would be to mock the String constructor using Jest; but I'm not quite sure how to do this.

@puradox
Copy link

puradox commented Apr 7, 2016

What's the status of this PR? I'm running into the same problem and would like to see this merged.

@leebyron
Copy link
Collaborator

leebyron commented Apr 8, 2016

Tests look good, just need to rebase and merge. Will get to that shortly.

@dashed
Copy link
Contributor Author

dashed commented Apr 8, 2016

This needs more work. I won't be able to look into it for the next few weeks...

@puradox
Copy link

puradox commented Apr 8, 2016

@dashed Can you explain what needs more work? I can lend a hand 👍

@dashed
Copy link
Contributor Author

dashed commented Apr 8, 2016

@puradox #620 (comment)

@leebyron leebyron merged commit 95b65bd into immutable-js:master Apr 18, 2016
@dashed
Copy link
Contributor Author

dashed commented Apr 20, 2016

Yay! Thanks @leebyron ! 🎉

@runru
Copy link

runru commented Jan 21, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants