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

Implement ResponseConstructor to allow overrides from other runtimes #1941

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

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

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.

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.

5 participants