-
Notifications
You must be signed in to change notification settings - Fork 438
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
Implement ResponseConstructor
to allow overrides from other runtimes
#1941
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. |
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.
Please regenerate the baselines
IMO, there should be more overall changes, not just special treatment of |
@microsoft-github-policy-service agree |
Agree, but wasn't sure which ones should be added, and it's sort of a pandoras box to some extent. The "real fix" could actually be to make the generator emit My suggestions would be:
|
Looks good to me. In addition, I would like to do the same for Another concern is that the current way of specifying |
interface ResponseConstructor { | ||
} |
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.
The Audio Worklet type has been affected.
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.
Why is this the case? Because Response was declared as exposed in Worker?
@saschanaz My understanding is that class merging is not an as-complete solution. For example when merging static methods (e.g. a Additionally could it makes sense to have this merge now in the interim anyway? Until classes can be emitted. Would be great as this is blocking us at the moment. |
As stated, this will block migration to classes if people start to depend on the interim way. |
I see, so microsoft/TypeScript#2957 has to move on... |
Got it, thanks for clarifying, @saschanaz. Would love to get this working, so have asked about the feasibility of an ambient-only implementation of this behaviour in the TypeScript issue linked above. |
+1 to not implementing a divergent declaration paradigm for a few arbitrarily-selected interfaces. |
This PR converts
Response
to be declared as the interfaceResponseConstructor
This allows for types from some runtimes to implement extra overloads for the Response class and static methods
For example, Bun supports
Response.redirect(location, statusOrRequestInit)
. Bun's types cannot declarevar Response
because if a user needs lib.dom loaded then the declarations clash, and typechecking failsResponse
is currently declared asvar Response
, which means we cannot use interface merging to append these extra propertiesI'm not sure if this is the best approach for modifying/overriding the types. This was implemented by dumping the webidl JSON and then copying in the overrides for making a ResponseConstructor exist
There was also an existing issue (#1518) that asked for this