Skip to content
This repository has been archived by the owner on Feb 7, 2023. It is now read-only.
This repository has been archived by the owner on Feb 7, 2023. It is now read-only.

RFC: Exportable types #295

Closed
penalosa opened this issue Oct 17, 2022 · 7 comments
Closed

RFC: Exportable types #295

penalosa opened this issue Oct 17, 2022 · 7 comments
Labels
v4 For the next major version

Comments

@penalosa
Copy link
Contributor

penalosa commented Oct 17, 2022

One request we've had is to provide user importable types from this package—i.e. a way to use worker types without declaring it in tsconfig.json. This is easy enough for most of our types (ExecutionContext, for instance):

// types.d.ts
export interface ExecutionContext {
  waitUntil(promise: Promise<any>): void;
  passThroughOnException(): void;
}
// index.ts
import { ExecutionContext } from "./types.d.ts"

The rest of this issues will address options for providing importable types for globals like Response, Request, and fetch, which are both values and types.

Is this a problem to solve?

The workers runtime does have the globals Response etc..., and so in some sense using ambient declarations is the "right" approach (as this package does currently). However, there are downsides, including the lack of automatic inclusion of @types/* packages, and the fact that workers can't be typed correctly in frameworks like Remix, which mix browser code and worker code in the same project (and even file!).

Here's a very simple example of a Remix file that doesn't typecheck, because Response.json() is non standard. It runs fine when published, as Response.json() is available in the workers runtime.

import { useLoaderData } from '@remix-run/react';

export const loader = async () => {
  return Response.json({
    data: [
      {
        slug: 'my-first-post',
        title: 'My First Post'
      }
    ]
  });
};
export default function Index() {
  const { data } = useLoaderData();
  return <div>{JSON.stringify(data)}</div>;
}

How about just...exporting the types?

At first glance, just adding an export keyword would seem to solve the issue:

// types.d.ts
export declare class Response extends Body {
   ...
}
// index.ts
import { Response } from "./types.d.ts"

Indeed, this typechecks fine, but doesn't build fine (esbuild can't find the Response export).

An honestly pretty reasonable option for classes

// types.d.ts
declare class Response extends Body {
  ...
}
class WorkerResponse extends Response {}

export {WorkerResponse as Response}
// index.ts
import { Response } from "./types.d.ts"

This both typechecks and builds with esbuild, as well as (as far as I can tell) working completely fine in the runtime.

It also provides a solution for the Remix example given earlier:

```ts
import { useLoaderData } from '@remix-run/react';
import { Response as WorkerResponse } from './types.d.ts';

export const loader = async () => {
  return WorkerResponse.json({
    data: [
      {
        slug: 'my-first-post',
        title: 'My First Post'
      }
    ]
  });
};
export default function Index() {
  const { data } = useLoaderData();
  return <div>{JSON.stringify(data)}</div>;
}

Frankenstein's monster

One type remains; the function:

declare function fetch(
  request: Request | string,
  requestInitr?: RequestInit | Request
): Promise<Response>;

I propose the following solution:

// types.d.ts
declare function fetch(
  request: Request | string,
  requestInitr?: RequestInit | Request
): Promise<Response>;

// @ts-ignore next-line
const WorkerFetch: typeof fetch = globalThis.fetch

export {
  WorkerFetch as fetch
}

// @ts-ignore next-line is needed because globalThis isn't typed. Can we type globalThis, you may ask? No, since that would inject fetch as a global type, which is exactly what this approach is trying to avoid.

This typechecks, builds, and runs correctly, but is a bit...horrifying. If anyone has any suggestions on how to improve this I'm all ears!

@GregBrimble
Copy link
Member

Does Frankenstein's approach work for the other types as well? Any reason not to do that and be consistent on everything?

@GregBrimble
Copy link
Member

Additionally, can we keep the global types for those that want to continue using workers-types in the v2 way?

@penalosa
Copy link
Contributor Author

penalosa commented Oct 17, 2022

@GregBrimble

  1. No, because a class is both a type and a value, whereas const Response = globalThis.Response is just a value. To get the type of Response, you'd need to use typeof Response, which breaks the usual expectation for classes. That's an okay tradeoff for functions, because you need typeof for functions anyway (I'm pretty sure, at least).
  2. Yes, this would be just another option

@penalosa
Copy link
Contributor Author

If we publish an ambient index.d.ts and an importable index.ts, existing code should keep working, and you'll also be able to import. Something to consider (cc @mrbbot), do we also want to try and publish to a @types/cloudflare-workers or something like that?

@GregBrimble
Copy link
Member

Hey @jacob-ebey, we're thinking about publishing importable Workers runtime types, as well as continuing to offer the global types. Do you have any thoughts on this? Would there be interest in using the importable types in the Remix packages/templates? Or does it make sense only for users to use directly themselves?

@mrbbot
Copy link
Contributor

mrbbot commented Oct 21, 2022

@mrbbot mrbbot added the v4 For the next major version label Oct 21, 2022
@mrbbot
Copy link
Contributor

mrbbot commented Jan 19, 2023

I'm going to close this, as you can now import type { ... } from "@cloudflare/workers-types" as of @cloudflare/workers-types version 4.

@mrbbot mrbbot closed this as completed Jan 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
v4 For the next major version
Projects
None yet
Development

No branches or pull requests

3 participants