jaredwray / keyv

Simple key-value storage with support for multiple backends
https://keyv.org
MIT License
2.61k stars 143 forks source link

Keyv no longer support generic parameter? #1128

Closed tatejones closed 1 month ago

tatejones commented 1 month ago

Describe the bug Previously the keyv constructor supported a generic parameter. No documentation on the migration. I assume you need to update the set and get methods

How To Reproduce (best to provide workable code or tests!) Version 4 code won't compile.

jaredwray commented 1 month ago

@tatejones can you send me an example code of this, and V5 and the expected result?

tatejones commented 1 month ago

Previous the constructor allowed this.

const store = new Keyv({....})

The Keyv doesn't allow this anymore, hence the get will not reflect this as the returned type.

In v5 you can supply the type on the get. This assumes the store has persisted different types and the client can infer the type expected from the key.

const someType = keyv.get(....)

I had to created a proxy around keyv that takes the generic type on the constructor and provides it on the get and set (not any for value). Is there a reason why this removed from the constructor with a default?

jaredwray commented 1 month ago

We did some changes on the constructor as to not allow the uri to be passed but any storage adapter should work and also the KeyvOptions can be passed. You can see that here:

This is passing in the store options: https://github.com/jaredwray/keyv/blob/69d89e8adf39c84f96f741c976b1f9de0826039c/packages/keyv/test/test.ts#L28

Here we are just passing in the store with no options: https://github.com/jaredwray/keyv/blob/69d89e8adf39c84f96f741c976b1f9de0826039c/packages/keyv/test/test.ts#L46

You also see that on get it should return the value just like before in this unit test:

https://github.com/jaredwray/keyv/blob/69d89e8adf39c84f96f741c976b1f9de0826039c/packages/keyv/test/test.ts#L46

tatejones commented 1 month ago

Not concerned about the options.The constructor doesn’t allow a generic type anymore.   What is the point of having them on the get when it isn’t bounded to anything?  On the @. 11 Sep 2024, at 9:50 AM, Jared Wray @.> wrote: We did some changes on the constructor as to not allow the uri to be passed but any storage adapter should work and also the KeyvOptions can be passed. You can see that here: This is passing in the store options: https://github.com/jaredwray/keyv/blob/69d89e8adf39c84f96f741c976b1f9de0826039c/packages/keyv/test/test.ts#L28 Here we are just passing in the store with no options: https://github.com/jaredwray/keyv/blob/69d89e8adf39c84f96f741c976b1f9de0826039c/packages/keyv/test/test.ts#L46 You also see that on get it should return the value just like before in this unit test: https://github.com/jaredwray/keyv/blob/69d89e8adf39c84f96f741c976b1f9de0826039c/packages/keyv/test/test.ts#L46

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

tatejones commented 1 month ago

I noticed the first post didn’t include the example from v4 with the generic type due to the markup removing it.

const foobarStore = new Keyv< FooBar >(options)

all it needs is a default and everything will continue to work.

class Keyv(….)

jaredwray commented 1 month ago

@tatejones - thanks. Do you want to send in a pull request with this change so I can review?

tatejones commented 1 month ago

Ok.  I will schedule it for next week. At the moment on leave

wrote: @tatejones - thanks. Do you want to send in a pull request with this change so I can review?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

tatejones commented 1 month ago

By adding the generic type to the keyv class this will have an impact on the api that will cause compiling issues for clients using the generic type on the get or iterator.

class Keyv<Value = any> extends EventManager {
.....
}

This means all the get's need to have the generic type Value removed as this means nothing as they will always return the type Value. Very similar pattern to the Set interface.

from

    async get<Value>(key: string, options?: {raw: false}): Promise<StoredDataNoRaw<Value>>;

to

    async get(key: string, options?: {raw: false}): Promise<StoredDataNoRaw<Value>>;

The set and iterator will also need changing.

jaredwray commented 1 month ago

@tatejones - looks like a PR is in for this: https://github.com/jaredwray/keyv/pull/1139

tatejones commented 1 month ago

The set and iterator are missing the generic type and the get doesn’t require one as it is can be inferred from the constructor on the return.On the @. 30 Sep 2024, at 3:11 AM, Jared Wray @.> wrote: @tatejones - looks like a PR is in for this: #1139

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

jaredwray commented 1 month ago

@tatejones - this will be released within the next 2-3 weeks.