KV `get()` and `getWithMetadata()` parameters are wrong

[GregBrimble]

Existing documentation URL(s)

https://developers.cloudflare.com/kv/api/read-key-value-pairs/

What changes are you suggesting?

env.NAMESPACE.get(key: string, type?: string, options?: { cacheTtl: number; type: string });

is wrong. It's actually:

env.NAMESPACE.get(key: string, options?: string | { cacheTtl?: number; type?: string });

https://github.com/cloudflare/workerd/blob/a8c1bf917d59128c3ba680faa1fb44a05c9c5a20/src/workerd/api/kv.c%2B%2B#L108

Similarly, it should be:

env.NAMESPACE.getWithMetadata(key: string, options?: string | { cacheTtl?: number; type?: string });

https://github.com/cloudflare/workerd/blob/a8c1bf917d59128c3ba680faa1fb44a05c9c5a20/src/workerd/api/kv.c%2B%2B#L119

Additional information

I haven't checked any other pages. Might also be mistakes there. Worth considering some dynamic gen here too, like the Wrangler commands? cc. @KianNH

[thomasgauvin]

Has this changed recently? If so, the person making changes to the binding should also be updating docs. Last I checked, type is a top level parameter as well.

Type within the object is not optional

Same for getWithMetadata

The screenshots are from types gen for the bindings that I'm hosting here: https://workers-bindings-typedoc.pages.dev/interfaces/KVNamespaceGetOptions#Type they might be out of date

[thomasgauvin]

Created #18303 to make cacheTtl optional

[GregBrimble]

This has not changed recently, no. It's been this way for years. You can indeed pass a type string as the second param. That's present in my suggestion (options?: string | { cacheTtl?: number; type?: string }). It's either a string for the type, or it's an object with optional cacheTtl and type props.

type looks optional to me: https://github.com/cloudflare/workerd/blob/e789b3a6eddfc4f997318414dd39ee1d6d86a3df/src/workerd/api/kv.h#L42. Try it out in a real Worker as see?

[thomasgauvin]

This has not changed recently, no. It's been this way for years. You can indeed pass a type string as the second param. That's present in my suggestion (options?: string | { cacheTtl?: number; type?: string }). It's either a string for the type, or it's an object with optional cacheTtl and type props.

type looks optional to me: https://github.com/cloudflare/workerd/blob/e789b3a6eddfc4f997318414dd39ee1d6d86a3df/src/workerd/api/kv.h#L42. Try it out in a real Worker as see?

I see. I don't love this parameter overloading, makes it difficult to clearly communicate in the docs. I'll opt for documenting both separately

[Oxyjun]

Hey @thomasgauvin, is there outstanding work for this issue?

[thomasgauvin]

Yes, this was merged