search

Level / level-js

An abstract-leveldown compliant store on top of IndexedDB.
MIT License
542 stars 44 forks source link

fix: allow empty prefix option #184

Closed achingbrain closed 4 years ago

achingbrain commented 4 years ago

This PR changes the default behaviour of the constructor to allow empty prefixes in databases - previously if you passed '' as the prefix it would set it to level-js-.

vweevers commented 4 years ago

Could this be considering a breaking change?

achingbrain commented 4 years ago

Could this be considering a breaking change?

I think this fixes a bug - the docs say:

The database name will be prefixed with options.prefix

which was not the case if you used '' as the prefix so I'm not sure that it's a breaking change.

vweevers commented 4 years ago

It breaks the expectation one might have that an empty prefix is treated as no prefix.

achingbrain commented 4 years ago

True, but thats not the expectation I had 🤣

achingbrain commented 4 years ago

Wait, hang on - what do you mean by 'empty' here? My expectation was that passing an empty string '' as the prefix would cause the database name to be prefixed with '', but it was being prefixed with level-js- and it seems with the current implementation there's no way to not have a prefix.

If I passed undefined (or null, to a lesser degree) I would expect the default to be set.

vweevers commented 4 years ago

and it seems with the current implementation there's no way to not have a prefix

That's true. Your use case (of not having a prefix) didn't come up before.

vweevers commented 4 years ago

In that sense it's a new feature so I'm making this semver-minor, be on the safe side.

vweevers commented 4 years ago

I'm splitting hairs now, but after reading https://github.com/ipfs/js-ipfs-repo/pull/215 I see that level-js did support an empty prefix before (courtesy of IDBWrapper). So, semver-patch.

@achingbrain ipfs is currently on level-js@4 (pending https://github.com/ipfs/js-datastore-level/pull/23) so would you like for this PR to be backported to 4x?

vweevers commented 4 years ago

5.0.1

achingbrain commented 4 years ago

would you like for this PR to be backported to 4x?

No, I think we should upgrade to the latest version of level*.

vweevers commented 4 years ago

@achingbrain Doesn't that defeat the point of these backward compat fixes, seeing as level-js@5 can't read old databases (unless they used binary keys)?

achingbrain commented 4 years ago

Fair point, maybe a backport would be nice 😄

vweevers commented 4 years ago

Alternatively you can use the db.upgrade() utility. LMK what you prefer.

achingbrain commented 4 years ago

I think we only use string keys and buffer values so that should be ok, right? UPGRADING.md says binary keys only and other types will be stringified?

Also is there any performance degradation to running db.upgrade on an already upgraded database?

vweevers commented 4 years ago

No, string keys of old databases will sort incorrectly. IndexedDB sorts by type in addition to "content" (by which I mean the raw bytes), so the key 'foo' is distinct from the key Buffer.from('foo').

To get around that - and be compatible with how leveldown behaves - level-js@5 converts string keys to binary keys before passing them to IndexedDB, so that string and binary keys are treated as the same. I.e. it effectively no longer sorts by type.

Also is there any performance degradation to running db.upgrade on an already upgraded database?

Yes, because it doesn't check whether the database is already upgraded. Every time you call db.upgrade() it visits all keys.

achingbrain commented 4 years ago

Ok, in that case, yes please on the backport!

We'll handle the key migrations in the https://github.com/ipfs/js-ipfs-repo-migrations when it's ready.

vweevers commented 4 years ago

Working on the backport - but browser tests are failing in 4xx for an unrelated reason. Will update.

vweevers commented 4 years ago

Backport landed in 4.0.2 (npm i level-js@latest-4).