fix: allow empty prefix option

[achingbrain]

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]

Could this be considering a breaking change?

[achingbrain]

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]

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

[achingbrain]

True, but thats not the expectation I had 🤣

[achingbrain]

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]

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]

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

[vweevers]

I'm splitting hairs now, but after reading ipfs/js-ipfs-repo#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 ipfs/js-datastore-level#23) so would you like for this PR to be backported to 4x?

[vweevers]

5.0.1

[achingbrain]

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

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

[vweevers]

@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]

Fair point, maybe a backport would be nice 😄

[vweevers]

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

[achingbrain]

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]

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]

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]

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

[vweevers]

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