Remove /// <reference lib="dom" /> from fetch-types.ts

[huw]

Problem

I am running the package inside Cloudflare Workers, and I’m using types: ["@cloudflare/workers-types"] to import Cloudflare’s type definitions. This imports a Cloudflare-specific Request type, which is mostly compatible with the lib.dom.d.ts Request, except in edge cases. The problem is that this package uses a triple-slash reference to re-import lib.dom.d.ts:

notion-sdk-js/src/fetch-types.ts

Line 1 in ab29303

/// <reference lib="dom" />

The effect is that any package which directly or indirectly relies on @notionhq/client uses Request from lib.dom.d.ts, and not the one from @cloudflare/workers-types, which is the one I want. This causes a handful of frustrating type errors due to the incompatibilities between the two.

Why is it this way right now?

The way the fetch-types.ts file is written suggests that the package is trying to be lenient about which fetch function the user brings with them, so it takes a union over the browser & Node fetches. However, we can’t write import { fetch } from "lib.dom.d.ts" to get just that type. We have a handful of options:

1. Add DOM to tsconfig.json’s types
2. Add /// <reference lib="dom" /> to fetch-types.d.ts
3. Write our own fetch declaration
4. Find another library which has a relatively up-to-date fetch declaration and import it

What’s the trade-off? (Updated 2023-07-10)

We’ve decided against (1) in the past in order to maintain the isomorphic nature of the library, in the sense that it can be imported from any TypeScript context and is self-containing. If we included DOM, users would be required to bring their own versions of the types that are implicitly imported (in this case, fetch) in their own tsconfig.jsons. This would most commonly be done by including DOM themselves, but users in other environments could include @cloudflare/workers-types or @types/node, for example (which they would be doing anyway in order to run in whichever environment they’re compiling to). However, @types/node doesn’t include a global fetch definition and may not for some time (DefinitelyTyped/DefinitelyTyped#60924).

tRPC solves this issue by importing RequestInfo/RequestInit, which are present in @types/node and reconstructing the fetch function.

(2), which the library does today, pollutes the global namespace as discussed above.

I’m writing this PR because I don’t think (2) makes as much sense as it used to and I want to start that conversation; as other runtimes proliferate and pre-18 versions of Node become more scarce, the trade-off in user support is swinging more toward (1). Arguably, long-term we should do (3) or (4) and I’d be happy to PR those also if the maintainers prefer :)

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ huw
❌ garrettheel
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[garrettheel]

hey @huw, thanks for putting together such a thorough analysis. the problem makes total sense. options (3) or (4) seem best to me, in that they shouldn't break any existing consumers. if you'd be willing to implement one of those then I'm happy to get this merged for you! let me know if that looks like a larger lift, and we can discuss alternatives

[huw]

I went back and forth a bit on the best implementation, but since this library hits a relatively small surface area on fetch, it was a small enough lift to just fully type the fetch function using primitives. The only non-primitive values I used to construct the types are Promise (ES2018) and Record (ES5), which both fall under ES2019.

One other exception: Agent, which the library was already importing from Node. Suffice to say that the library is no more or less self-contained than it already was and that it can still be imported in multiple runtimes.

I then decided to keep the existing assertion tests we had in place, which meant adding DOM to lib in tsconfig, in order to allow us to check against those types. However, because the DOM types are only used in the assertion tests, and the tests are stripped from the final package, this won’t introduce a new implicit dependency on consumers. It will mean that contributors to this repo will have to be more careful about using types that might introduce such a dependency in the future; I’ll leave it up to you to decide whether that trade-off is worth having the assertion test in place.

I also added assertion tests for node-fetch and @cloudflare/workers-types which do the same thing. I wasn’t able to add assertion tests for other popular runtimes (as measured by State of JS), as we require those packages to export their types.

[garrettheel]

What do you think about just redefining the types inline, similar to https://github.com/michaelcpuckett/activity-kit/blob/2119ea8e95fde451d5057d7bd6d95330de34c15e/packages/types/lib/adapters/FetchPolyfill.d.ts? I think it's a little confusing to leak refs from the cloudflare package in source (even though it's only a devDependecy)

[garrettheel]

Approach generally looks OK to me - but I have one question about the workers-types dependency

[garrettheel]

Could we just declare this ourselves (declare let fetch: (input: RequestInfo, init?: RequestInit) => Promise<Response>;) for the assert on fetch-types.ts:74, or is this still necessary?

[huw]

RE both questions; the reason to do it this way is to keep the assertions up to date if the underlying libraries change. If we wanted to avoid this I’d just remove the relevant assertions entirely; there’s not much point in asserting that 1 === 1.

If I were writing this library I’d drop the other 2 assertions (keeping the node-fetch ones because they don’t have downsides) and just be mindful if the fetch spec ever updates (rare and unlikely to affect the subset we use), but given that we already had assertions in the codebase I wasn’t sure if the repo’s stance was to keep those tests in place. LMK your take and I’ll update the PR :)

[garrettheel]

I agree with the way you'd write it and low likelihood that the a fetch spec change would break this (or that @types/node will give us a better way to represent this by the time it does).

One question though - if we go this route, we can't rely on the node-fetch types to be compatible with DOM fetch right? AFAICT it's easy to update headers via NonNullable<Record<string, string>> but body is harder since ReadableStream | XMLHttpRequestBodyInit has no equivalent node types. I might be missing something there though

[huw]

I’ve removed the two extra assertions and added some clarifying comments ^_^

You are correct—node-fetch, DOM fetch, native Node fetch (via undici), Cloudflare fetch, etc. are all incompatible on the margins. Here’s an insightful comment that explains some of it. We just have to hope that future contributors only the common subset of fetch features across implementations (which, as above, is quite large and unlikely to regularly change), or, as we do with Agent, make it optional.

[garrettheel]

Got it, I see how you solved body (which was my primary concern) and left one comment on that part. In agreement on the rest of it!

[garrettheel]

Is there any harm to also including the node-fetch body types (i.e. NodeRequestInit["body"]) here too? I'm concerned that there might be folks using Blob or stream types via node here that this breaks

[huw]

The short answer is no.

The long answer is (my apologies if I’m explaining stuff you already know): TypeScript uses duck typing, which means that as long as the user-provided fetch implementation accepts body: string, it doesn’t matter what else their fetch implementation accepts. We’re just specifying the minimal contract of things we need to use, which in our case is just string | undefined since in Client.ts, the only place fetch is used, we pass in the result of JSON.stringify() or undefined. For example, tRPC accept custom fetch implementations but only type a subset of body, which corresponds with what they actually use internally.

However, in practice, extending our contract wouldn’t hurt. Right now we type body: NonNullable<RequestInit["body"]> & NonNullable<NodeRequestInit["body"]>, which uses an intersection over the DOM and node-fetch types. This computes to something like body: Blob | URLSearchParams | FormData | string (I’m hazy on this, but the point is that, as an intersection, it removes anything that’s not the same type between implementations, such as NodeJS.ReadableStream). In this PR, taking the intersection over string & NodeRequestInit["body"] would compute to string, which is kind of pointless (but doing string | NodeRequestInit["body"] would cause all manner of incompatibilities because we’d require every client to accept NodeJS.ReadableStream, to give an example).

So I think it wouldn’t make sense to add this.

[garrettheel]

Thanks for the explanation! I had missed that we don't allow the user to pass through a body, and always pass through the result of JSON.stringify() -- so makes sense why keeping string is perfectly fine.

[huw]

FWIW in the process of answering some of those tests I cooked up a few MWEs on my machine and made sure that the types are still compatible with DOM, undici (native Node), node-fetch, @cloudflare/workers-types and @edge-runtime/types (which extends DOM).

I am about as confident as I can be that this won’t cause problems with the types & as such I think this should be a bugfix release (especially because it doesn’t touch the actual implementation).

[garrettheel]

Looks great, thanks again for making this change and helping me understand the implications to ensure it's safe! I'll include this in a patch release shortly.