Add [Symbol.toStringTag] property to all interfaces #1699
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. |
|
I made an attempt but I'm hoping to receive a bit guidance and early feedback on the approach and how to proceed. Running tests results in errors that look like: I'm not sure how to reconcile it. |
There was a problem hiding this comment.
You are already going in the right direction! One problem is that, as the error message says, Symbols are ES2015+ while dom.generated.d.ts needs to be ES5-compatible as of now. We probably need another set of files e.g. *.stringtag.d.ts just like *.iterable.d.ts.
|
@microsoft-github-policy-service agree |
|
Thanks for the feedback. I took your advice and tried to follow the existing iterable pattern. |
src/build/emitter.ts
Outdated
| @@ -135,7 +135,7 @@ function isEventHandler(p: Browser.Property) { | |||
| export function emitWebIdl( | |||
| webidl: Browser.WebIdl, | |||
| global: string, | |||
| iterator: "" | "sync" | "async", | |||
| flavor: "" | "sync" | "async" | "toStringTag", | |||
There was a problem hiding this comment.
sync/async is now confusing in this case, maybe iterator/asyncIterator?
src/build/emitter.ts
Outdated
|
|
||
| const interfaces = getElements(webidl.interfaces, "interface"); | ||
| interfaces.sort(compareName).forEach((i) => { | ||
| if (!i.noInterfaceObject && !i.namespace && !i.extends && !i.implements) { |
There was a problem hiding this comment.
I get i.extends as it'll be inherited from parent interface, but why other checks? BTW we do need to filter out i.legacyNamespace and emit it inside a namespace, see also:
TypeScript-DOM-lib-generator/src/build/emitter.ts
Line 1491 in 6621974
| readonly: true, | ||
| comment: | ||
| "The well-known symbol @@toStringTag.\n\n" + | ||
| "[MDN Reference](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Symbol/toStringTag)", |
There was a problem hiding this comment.
This is causing quite some size in the generated file, but I think the doc should be in the Symbol.toStringTag rather than in the function. IDEs probably won't be able to show the comment for functions named with symbols anyway.
So I think it's better to not have this comment.
|
I don't much like the implementation since it requires so many new files. Also, I don't understand how this is useful to anybody. It's not a property that you need to refer to directly, is it? Following the links in the original bug and skimming through them, it seems like there's some complex assignability problems without this property. Would it be possible to fix those in a simpler way (or stop using this property entirely -- I don't think it's meant for @saschanaz I skimmed the webidl spec discussion but couldn't find a definitive place that says that these are part of the DOM. How important do you think it is for spec compliance to have them here? |
By DOM I guess you meant Web Platform? Anything in Web IDL spec is part of Web Platform in that case, otherwise it won't be there. About the size, given that the new files are pretty much duplicated with same symbolic member definitions, perhaps there can be an empty |
|
About how important, I think it's pretty much edge case to use this at all, so maybe not too much worth extra effort 🤔 |
| const iterators = emitWebIdl(exposed, options.global[0], "sync"); | ||
| const toStringTag = emitWebIdl(exposed, options.global[0], "toStringTag"); | ||
| await fs.writeFile( | ||
| new URL(`${options.name}.tostringtag.d.ts`, options.outputFolder), |
There was a problem hiding this comment.
| new URL(`${options.name}.tostringtag.d.ts`, options.outputFolder), | |
| new URL(`${options.name}.tostringtag.generated.d.ts`, options.outputFolder), |
|
Just a heads up. I'm not actively working on this at the moment. If someone would like to pick up the remaining work to take it over the finish line, that is welcome. |
Fixes #1641