Skip to content

Conversation

@alii
Copy link

@alii alii commented Mar 15, 2025

This PR converts Response to be declared as the interface ResponseConstructor

This allows for types from some runtimes to implement extra overloads for the Response class and static methods

interface ResponseConstructor {
  prototype: Response;
  new(body?: BodyInit | null, init?: ResponseInit): Response;
  redirect(url: string | URL, status?: number): Response;
  // ... more properties
}

var Response: ResponseConstructor

For example, Bun supports Response.redirect(location, statusOrRequestInit). Bun's types cannot declare var Response because if a user needs lib.dom loaded then the declarations clash, and typechecking fails

Response is currently declared as var Response, which means we cannot use interface merging to append these extra properties

I'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

@github-actions
Copy link
Contributor

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.

Copy link
Contributor

@Bashamega Bashamega left a 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

@petamoriken
Copy link
Contributor

IMO, there should be more overall changes, not just special treatment of Response. The reason is that ReadableStream.from is not typed yet because it is not implemented in two browsers, but Deno is currently patching it. There may be other examples.

@alii
Copy link
Author

alii commented Mar 16, 2025

@microsoft-github-policy-service agree

@alii
Copy link
Author

alii commented Mar 16, 2025

IMO, there should be more overall changes, not just special treatment of Response. The reason is that ReadableStream.from is not typed yet because it is not implemented in two browsers, but Deno is currently patching it. There may be other examples.

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 interface [Type]Constructor {} for all class-like types by default, and then we'd be able to remove the overrides this PR adds anyway.

My suggestions would be:

  • Include a RequestConstructor here, feels "correct" to have at-least both
  • Make no changes (so this can merge😄), and instead reference this issue in the future if similar changes need to be made

@petamoriken
Copy link
Contributor

Include a RequestConstructor here, feels "correct" to have at-least both

Looks good to me. In addition, I would like to do the same for ReadableSteam, but I think I will do it in a separate PR.

Another concern is that the current way of specifying "Response": null in removedTypes.jsonc does not reflect changes in browser compat data, so I feel it would be better to have a new config file such as exportConstructorTypes.jsonc (bikeshed).

Comment on lines +865 to +866
interface ResponseConstructor {
}
Copy link
Contributor

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.

Copy link
Author

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
Copy link
Contributor

I still think we should do #222, and this will block #222 (we won't be able to have both class declaration and var declaration together)

@alii
Copy link
Author

alii commented Mar 16, 2025

@saschanaz My understanding is that class merging is not an as-complete solution. For example when merging static methods (e.g. a Response.redirect overload).

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.

@saschanaz
Copy link
Contributor

As stated, this will block migration to classes if people start to depend on the interim way.

@petamoriken
Copy link
Contributor

I see, so microsoft/TypeScript#2957 has to move on...

@alii
Copy link
Author

alii commented Mar 16, 2025

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.

@Renegade334
Copy link
Contributor

IMO, there should be more overall changes, not just special treatment of Response.

+1 to not implementing a divergent declaration paradigm for a few arbitrarily-selected interfaces.

@petdomaa100
Copy link

petdomaa100 commented Nov 22, 2025

I recently found myself needing to override Response.json to only accept certain types. ResponseConstructor would have been a good solution, but it seems to have been rejected in favor of #222.

I'm not familiar with this project's internal roadmap, but #222 has been open for a very long time. Maybe consider merging this as a temporary solution.

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.

6 participants