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

Install @@toStringTag on namespace objects #873

Merged
merged 2 commits into from
Jul 23, 2020

Conversation

shvaikalesh
Copy link
Contributor

@shvaikalesh shvaikalesh commented Apr 25, 2020

Follow-up of #357.

This change aligns WebIDL namespace objects with ECMA-262 ones like Math and Atomics.

Chrome (84.0.4126.0) already implements this PR:

WebAssembly[Symbol.toStringTag] // => "WebAssembly"

cc @domenic


Preview | Diff

@domenic
Copy link
Member

domenic commented Apr 25, 2020

Interesting. I don't think we should do this; instead we can fix Chrome. Namespace objects here are patterned after those in the JS spec, such as Math and JSON, which do not have such toStringTags.

Note also that the other namespace objects on the platform, CSS and Math, do not have such toStringTags in Chrome. So I think this is just a Chrome bug.

@domenic
Copy link
Member

domenic commented Apr 25, 2020

Oh, I didn't click through your link. I see the JS spec is inconsistent, and Reflect has no toStringTag, but all the others do.

Hmm, so in that case probably we should align with the JS spec...

@domenic
Copy link
Member

domenic commented Apr 25, 2020

I filed tc39/ecma262#1970 to get some clarity here. We should probably wait for that to settle before going either way.

@shvaikalesh
Copy link
Contributor Author

Thank you for rapid feedback! I've removed WebAssembly[@@toStringTag] from WebKit's patch.

index.bs Outdated Show resolved Hide resolved
@domenic domenic added the do not merge yet Pull request must not be merged per rationale in comment label Apr 30, 2020
@domenic
Copy link
Member

domenic commented Jul 20, 2020

@shvaikalesh it looks like the plan is to have toStringTag on namespace objects after all, per tc39/ecma262#2057 (comment). Would you be up for finishing this PR, and also adding web platform tests? (For web platform tests we should test WebAssembly, console, and CSS.)

Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>
@ExE-Boss
Copy link
Contributor

When it comes to console, should the %Symbol.toStringTag% property be installed on console.[[Prototype]] or on console?

@domenic
Copy link
Member

domenic commented Jul 21, 2020

It should follow the normal rules for Web IDL.

@shvaikalesh
Copy link
Contributor Author

@domenic Thank you for moving this forward! The tests are at web-platform-tests/wpt#24717. Is there something I could do to finish this PR?

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

PR looks good! I'll merge. If you could also file issues on the engines per https://github.com/heycam/webidl#breaking-changes-should-be-filed-against that'd be ideal.

@domenic domenic merged commit d7145e2 into whatwg:master Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge yet Pull request must not be merged per rationale in comment
Development

Successfully merging this pull request may close these issues.

3 participants