-
Notifications
You must be signed in to change notification settings - Fork 452
Emit Symbol.toStringTag
properties in TS 6.0
#2193
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR! This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged. |
a4864a2
to
62a6dcf
Compare
}, | ||
"DOMException": { | ||
"extends": "Error" | ||
"extends": "Error", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move DOMException to a .kdl file, maybe move it to dom.kdl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving the existing override is really beyond the scope of this changeset; would be better suited to a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving the existing override is really beyond the scope of this changeset; would be better suited to a follow-up PR.
Then remove the changes in patches.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why noToStringTag at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error
doesn't have a toStringTag property in the TS libs, and DOMException is a common target for userland inheritance, so the concern is that this will do more harm than good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should test it in TS repo without noToStringTag - and if it gives some trouble then we should consider it. Right now it feels arbitrary.
(idk much about testing in TS repo, @jakebailey knows more)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's pretty easy to check these variants (though a TS team member has to send the PR as we auto close anyone else trying to send a PR updating these files due to people not understanding they shouldn't touch generated files in the repo 😄)
62a6dcf
to
2ea828a
Compare
2ea828a
to
28dd4e8
Compare
Fixes #1641.
Fixes #2172.
Alternative to #1762.
Supersedes #2075.
Reference: https://webidl.spec.whatwg.org/#dfn-class-string
This amends the builder to append
Symbol.toStringTag
properties to interfaces and iterators in the ts6.0 core libraries. (ES6's well-known symbols are universally defined in ts6.0.)Because adding a property with a literal type affects inheritance chains, only "final" classes (ie. those that do not have any inheritance children within the current global) can have these properties typed as string literals. (For example, it's not possible for
Blob#[Symbol.toStringTag]
to be typed as"Blob"
, because File inherits from Blob, and a property of type"File"
can't override a parent property of type"Blob"
.)For those interfaces that have one or more inheritance children, there are two possible approaches.
Symbol.toStringTag
properties, and others (including some very common ones) without them.string
, so that they can be correctly overridden by child interfaces. This is the approach taken here.The toStringTag emitter does not add to any interface marked
noInterfaceObject
, since these are usually pseudo-types that are not true IDL interfaces, nor to namespaces converted to interfaces (ie.console
).Also adds a
noToStringTag
interface property to explicitly suppress emitting aSymbol.toStringTag
property. This is useful for interfaces known to be a common target for userland inheritance, for exampleDOMException
, which would otherwise be emitted within audioworklet.