-
Notifications
You must be signed in to change notification settings - Fork 146
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
Next major version #80
Comments
out of curiosity, why not moving the example, instead of this: import { set, createStore } from 'idb-keyval';
const store = createStore('custom-keyval-db', 'custom-keyval-store');
set('hello', 'world', store); one could do this: import { createStore } from 'idb-keyval';
const { set } = createStore('custom-keyval-db', 'custom-keyval-store');
set('hello', 'world'); |
Object properties and class methods can't tree-shake https://www.youtube.com/watch?v=lsd2-TCgHEs |
and for a db case where you always either get or set values, is worse DX a win? but fair enough, although needing to keep passing the store as last argument doesn't compose too well, imho, maybe the ability to bind the import { set as defaultSet, createStore } from 'idb-keyval';
const set = defaultSet.bind(createStore('custom-keyval-db', 'custom-keyval-store'));
set('hello', 'world'); That guards against spread or any other kind of arguments related operations, but it's just a hint, and I don't think your proposed changes are a big deal anyway 👋 |
I don't think it's worse DX. There are issues open right now because folks can't rename stores, and can't integrate idb-keyval into their existing databases. I'd be taking something currently impossible with this library and making it possible, while the simple case remains just as simple. That sounds like a DX improvement to me. I don't think binding is a win vs: import { set as defaultSet, get as defaultGet, createStore } from 'idb-keyval';
const store = createStore('custom-keyval-db', 'custom-keyval-store');
const set = (...args) => defaultSet(...args, store);
const get = (...args) => defaultGet(...args, store);
set('hello', 'world'); |
definitively, if the simple case keeps being simple, which I believe is the only way I've used to date this library. however, I personally wouldn't mind the following, not caring about tree shaking ... it looks the cleanest, imho, but like I've said, your proposal works too, and arrow/bind/whatever seems like an easy thing to put on top 👍 import { createStore } from 'idb-keyval';
const { get, set } = createStore('custom-keyval-db', 'custom-keyval-store'); |
No complaints on this! Love the new API and the consideration of tree-shaking! Mostly I've been using the single store way. I wanted to add it to an existing one but didn't make a problem out of it. Super happy It will be possible. |
#74 reminded me that things get messy with multiple optional args. I should put the store into a options object consistently throughout the API, along with other optional arguments. |
Objects properties do not minify well, if you want to get on the same level as @developit. |
😀 usability comes before minification |
Any updates? |
No. I'm pretty busy right now, and it'll be late December / next year before I have time to dig into this. |
PR at #94 |
The stuff around setting the database & store name has been a bit of a failure. It bloats the library, and renaming stores is a bit of a mess. See the difficulties in #73.
Instead, I think it'd be better to allow developers to provide an object store.
The simple case will remain simple:
If you want to create a database with a custom name and a custom store name:
However, if you want to integrate idb-keyval with an existing database, or perform schema migrations on an existing database (like, renaming a store), you'll need to provide a store-getting function:
For example:
It feels like this keeps the simple cases simple, whilst allowing total control over the underlying database in more custom cases.
The library will become smaller for most users with this change.
More advanced use-cases are better-handled by https://www.npmjs.com/package/idb.
The text was updated successfully, but these errors were encountered: