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

feat: transform Request, Response, and WebSocket classes to interfaces with var declarations #2708

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

Conversation

andyjessop
Copy link

@andyjessop andyjessop commented Sep 13, 2024

This PR is part of the drive for Node.js compatibility and focusses on ensuring 3 specific interfaces, Request, Response, and WebSocket, are compatible with @types/node.

The maintainers of @types/node considered a related use case when adding fetch types to Node, because it's relatively common for lib.dom and @types/node to be loaded together, which would cause similar conflicts to what we're seeing now.

Because of that, they use this pattern when defining a web type:

interface Response extends _Response {}
  var Response: typeof globalThis extends {
      onmessage: any;
      Response: infer T;
  } ? T
      : typeof import("undici-types").Response;

This defers to the global Response type if it's available, falling back to undici otherwise.

In order to get this to work for us, we need to declare declare var onmessage: never, then Response will be our Response.

Next, we need to get these three interfaces onto globalThis. In order to do that, our class delcarations need to be converted to var declarations. And in order to get the same type of behaviour (e.g. in return types), we need a corresponding interface. For example:

interface Request<
  CfHostMetadata = unknown,
  Cf = CfProperties<CfHostMetadata>,
> extends Body {
  ...
}
declare var Request: {
  prototype: Request;
  new(input: RequestInfo | URL, init?: RequestInit): Request;
};

Implementation

This PR creates two transformers that will transform our types in the worker:

  1. createAddOnMessageDeclarationTransformer - adds the onmessage declaration.
  2. createClassToInterfaceTransformer - converts our classes to an interface+var declaration pair.

Copy link


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


Andy Jessop seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

types/src/index.ts Show resolved Hide resolved
types/src/transforms/class-to-interface.ts Outdated Show resolved Hide resolved
types/src/transforms/class-to-interface.ts Show resolved Hide resolved
types/src/transforms/onmessage-declaration.ts Outdated Show resolved Hide resolved
@penalosa
Copy link
Collaborator

This looks good to me! It would be great to see the diff of types generated with this turned on vs with this turned off, to verify the behaviour is correct

@andyjessop
Copy link
Author

@penalosa This gist shows a comparison of the new vs old generated types: https://gist.github.com/andyjessop/ca8456d22b2abb44542143d55c196040

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.

2 participants