[CloudflareKVPlugin]: Incompatible types when passing in argument for kv

[AdiRishi]

Describe the bug

      const kvCache = createKvCache({
        KV: env.GRAPHQL_RESPONSE_CACHE,
        ctx,
        keyPrefix: 'graphql', // optional
        cacheReadTTL: 1000 * 60 * 60 * 4, // 4 hours
      });

When using the createKvCache export from @envelop/response-cache-cloudflare-kv the passed in type of KV is rejected with this error

src/index.ts:29:9 - error TS2322: Type 'KVNamespace<string>' is not assignable to type 'import("/Users/arishi/personal/sukrit-ecosystem-turborepo/node_modules/.pnpm/@[email protected]/node_modules/@cloudflare/workers-types/index").KVNamespace<string>'.
  Types of property 'get' are incompatible.
    Type '{ (key: string, options?: Partial<KVNamespaceGetOptions<undefined>> | undefined): Promise<string | null>; (key: string, type: "text"): Promise<...>; <ExpectedValue = unknown>(key: string, type: "json"): Promise<...>; (key: string, type: "arrayBuffer"): Promise<...>; (key: string, type: "stream"): Promise<...>; (key:...' is not assignable to type '{ (key: string, options?: Partial<import("/Users/arishi/personal/sukrit-ecosystem-turborepo/node_modules/.pnpm/@[email protected]/node_modules/@cloudflare/workers-types/index").KVNamespaceGetOptions<undefined>> | undefined): Promise<...>; (key: string, type: "text"): Promise<...>; <ExpectedValue ...'.
      Types of parameters 'options' and 'type' are incompatible.
        Type '"stream"' has no properties in common with type 'Partial<KVNamespaceGetOptions<undefined>>'.

29         KV: env.GRAPHQL_RESPONSE_CACHE,
           ~~

  ../../node_modules/.pnpm/@[email protected]_@[email protected][email protected]/node_modules/@envelop/response-cache-cloudflare-kv/typings/index.d.ts:7:5
    7     KV: KVNamespace;
          ~~
    The expected type comes from property 'KV' which is declared here on type 'KvCacheConfig'


Found 1 error in src/index.ts:29

To Reproduce Steps to reproduce the behavior:

Expected behavior

The types should be compatible.

Environment:

• OS: MacOS
• NodeJS: 18.x
• "@envelop/response-cache": "^6.1.1",
• "@envelop/response-cache-cloudflare-kv": "0.2.0",

[EmrysMyrddin]

Coming back here. So the new snapshot is not working ? Are you sure you've cleaned up your node_modules and lockfile ?

Because it really looks like multiple version of the @cloudflare/workers-types are co-existing here

[AdiRishi]

Yeah I'm definitely inclined to agree with you here. I'm just going to make a clean test repo and see if this same issue exists, if not, I'll chalk it up to something weird with my setup.

[AdiRishi]

Alright, so I'm at a very weird point now. I've narrowed down the type error to the graphql-yoga package. Importing it seems to mess with the types for Cloudflare 🙃 😫

I've made a simple test repository - https://github.com/AdiRishi/worker-graphql-typecheck It contains both @envelop/response-cache-cloudflare-kv and cachified-adapter-cloudflare-kv (which is another CF package which accepts KV as an argument).

All the types work just fine. Uncomment line 2 at index.ts

 import { YogaInitialContext, createSchema, createYoga } from "graphql-yoga";

And now the types break with the same error as seen above 😕

@EmrysMyrddin if you have any idea on what's going on let me know. I thought maybe the @whatwg-node/fetch package which yoga depends on was causing issues due to node types, but I pinned @types/node to a working version and that's not the issue.

I'll keep investigating too 👍

[EmrysMyrddin]

Oh no 😒 I hate when TypeScript leads to this kind of problems...

One thing you can check is I know we are doing some type override in Yoga which have side effects on import.

[AdiRishi]

@EmrysMyrddin thanks for the tip, that helped a lot in the investigation! Here's what I've found.

In graphql-yoga/src/types.ts we have this little blob

declare global {
  interface ReadableStream<R = any> {
    [Symbol.asyncIterator]: () => AsyncIterator<R>;
  }
}

This explains the error quite well. We can see that typescript is complaining about something to do with the KVNamespace.get() and specifically something to do with stream. Looking at @cloudflare/workers-types we can see the following definitions for the stream variant of get

declare interface KVNamespace<Key extends string = string> {
  get(key: Key, type: "stream"): Promise<ReadableStream | null>;
  get(
    key: Key,
    options?: KVNamespaceGetOptions<"stream">
  ): Promise<ReadableStream | null>;
}

So, the error makes perfect sense now, graphql-yoga is changing the definition of ReadableStream which is itself defined within the @cloudflare/workers-types as follows

export interface ReadableStream<R = any> {
    readonly locked: boolean;
    cancel(reason?: any): Promise<void>;
    getReader(): ReadableStreamDefaultReader<R>;
    getReader(options: ReadableStreamGetReaderOptions): ReadableStreamBYOBReader;
    pipeThrough<T>(transform: ReadableWritablePair<T, R>, options?: StreamPipeOptions): ReadableStream<T>;
    pipeTo(destination: WritableStream<R>, options?: StreamPipeOptions): Promise<void>;
    tee(): [ReadableStream<R>, ReadableStream<R>];
    values(options?: ReadableStreamValuesOptions): AsyncIterableIterator<R>;
    [Symbol.asyncIterator](options?: ReadableStreamValuesOptions): AsyncIterableIterator<R>;
}

Now, this error only happens because in typical workers projects, it is common to setup @cloudflare/workers-types as global types as follows.

{
  "compilerOptions": {
    "types": [
      "@cloudflare/workers-types"
    ]
  }
}

It's the default way a workers project is setup when you do npm create cloudflare@latest

If you were to remove the global definition of @cloudflare/worker-types and just import the needed types directly, typescript won't complain.

@EmrysMyrddin how should we proceed here? I think the PR made should be released anyway, since it makes sense that @cloudflare/workers-types is a peer dependency. Not sure how to go about fixing this conflict. Maybe we can leave a note in the docs?

[EmrysMyrddin]

Let me check with the team why we do this black magic type declaration :-)

[EmrysMyrddin]

After some discutions, we definitely should not do this.

You can make a PR to remove this if you want :-)

[AdiRishi]

Alright, I'll give it a shot :)

Edit: I will likely be busy with personal work until the 18th of December. If I find time in between I may be able to finish this. But if not, expect some progress after the 18th.

[EmrysMyrddin]

Hey @AdiRishi! Did you have time to progress on this subject ?

[AdiRishi]

@EmrysMyrddin I did spend a little bit of time thinking about how I would fix the type. Honestly as far as I can tell the actual type in tslib is wrong (which is why the graphql-yoga packages tries to fix the type). The issue is that as soon as we remove that there are a lot of type errors that show up, and I don't have a good solution to fix it all, because...the code is actually doing the right thing.

Just to explain the above, let's take a look at the docs for ReadableSteam async iteration. As the docs clearly say, you can use any readable stream object with an async for iteration loop. HOWEVER, typescript doesn't allow this with the default types 🙃 🙃

I'd love to take your advice on this, not really sure how to go about fixing all the locations which would break as a result of removing that global override.

[EmrysMyrddin]

@enisdenjo Do you have an idea ? I'm not a TypeScript expert ^^'

[AdiRishi]

@EmrysMyrddin your bump on the docs PR got me to look into this again. I have a PR that does effectively remove the type override from the global namespace 🎉

[ardatan]

Instead of a magic with type casts and so on, we can align Yoga's override with CloudFlare types by making the following change;

declare global {
  interface ReadableStream<R = any> {
-    [Symbol.asyncIterator]: () => AsyncIterator<R>;
+    [Symbol.asyncIterator]: () => AsyncIterableIterator<R>;
  }
}

[ardatan]

Also see my PR to simplify cache initialization for CloudFlare workers or any other dynamic cache initialization; https://github.com/n1ru4l/envelop/pull/2238#issuecomment-2119483733

[ardatan]

Thanks for the feedback and work! I think #2238 solves it in a different way so we don't need to worry about the type conflicts.