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 support for Symbol.toStringTag (closes #21). #36

Closed
wants to merge 3 commits into from

Conversation

droooney
Copy link

No description provided.

@TimothyGu
Copy link
Member

This isn't actually compliant to the Web IDL spec, which says the prototype's @@toStringTag must be the interface's name + 'Proto'. But from @domenic in nodejs/node#10906 (comment):

Regarding toStringTag, this is actually a controversial part of the Web IDL spec. See https://www.w3.org/Bugs/Public/show_bug.cgi?id=28244. I would suggest following Chrome's version, of just putting it as a data property on the prototype, but I am somewhat biased there :).

Regardless, you probably should remove the existing prototype.toString function.

@domenic
Copy link
Member

domenic commented Apr 24, 2017

Yes, I am much happier with this than the version in the Web IDL spec, which nobody implements. But yes, we need to remove the toString function that already exists.

@droooney
Copy link
Author

Should I set a getter according to the specs (add 'Prototype' for the prototype) then (which seems controversial to me as well)?

@domenic
Copy link
Member

domenic commented Apr 25, 2017

No, as I said, I am much happier with this version than the version in the Web IDL specs. The only change is that we need to remove the toString function that we're currently installing on every class.

@droooney
Copy link
Author

No problem :) I personally like this version more as well.

@Sebmaster
Copy link
Member

We need to keep toString for the stringifier and impl toString method. Only the stringtag part should be removed.

@droooney
Copy link
Author

Oops, sorry... Ridiculously long commit history for such a small change :)

@@ -365,18 +365,15 @@ Interface.prototype.generateToString = function () {
}
}
if (!hasToString) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition is now flipped; it should be if (hasToString).

I'll fix this before merging.

@domenic
Copy link
Member

domenic commented Apr 29, 2017

I ended up doing this a bit differently; it turns out that whole generateToString() function is no longer needed. See 0590e5a. Thanks for the kick-start though!

@domenic domenic closed this Apr 29, 2017
@droooney
Copy link
Author

No problem :) Thanks for awesome jsdom!

@droooney droooney deleted the fix-21 branch May 2, 2017 10:53
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