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

Merged
merged 5 commits into from
Sep 30, 2024

Conversation

andyjessop
Copy link
Contributor

@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

github-actions bot commented Sep 13, 2024

All contributors have signed the CLA ✍️ ✅
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
Contributor Author

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

@andyjessop
Copy link
Contributor Author

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

github-actions bot added a commit that referenced this pull request Sep 30, 2024
@andyjessop andyjessop force-pushed the aj/class-to-interface-transformer branch from f76df21 to cfd832b Compare September 30, 2024 07:54
@andyjessop andyjessop merged commit 579048a into main Sep 30, 2024
14 checks passed
@andyjessop andyjessop deleted the aj/class-to-interface-transformer branch September 30, 2024 08:11
@philippviereck
Copy link

Could this have broken types for Node.js compatibility like: import { Buffer } from 'node:buffer'; ?

@penalosa
Copy link
Collaborator

penalosa commented Oct 2, 2024

Could this have broken types for Node.js compatibility like: import { Buffer } from 'node:buffer'; ?

Currently @cloudflare/workers-types doesn't include any types for Node.js compat, and we recommend installing @types/node@20.8.3 alongside @cloudflare/workers-types in your Worker. This PR makes @cloudflare/workers-types compatible with more up-to-date versions of @types/node, and shouldn't affect typing of things like import { Buffer } from 'node:buffer'; in your project.

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.

3 participants