Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keyv no longer support generic parameter? #1128

Open
tatejones opened this issue Sep 9, 2024 · 8 comments
Open

Keyv no longer support generic parameter? #1128

tatejones opened this issue Sep 9, 2024 · 8 comments
Labels

Comments

@tatejones
Copy link
Contributor

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.

@tatejones tatejones added the bug label Sep 9, 2024
@jaredwray
Copy link
Owner

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

@tatejones
Copy link
Contributor Author

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
Copy link
Owner

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:

const store = new Map();

Here we are just passing in the store with no options:

const store = new Map();

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

const store = new Map();

@tatejones
Copy link
Contributor Author

tatejones commented Sep 10, 2024 via email

@tatejones
Copy link
Contributor Author

tatejones commented Sep 11, 2024

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<Value = any>(….)

@jaredwray
Copy link
Owner

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

@jaredwray jaredwray reopened this Sep 11, 2024
@tatejones
Copy link
Contributor Author

tatejones commented Sep 11, 2024 via email

@tatejones
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants