Update to abstract-leveldown 4 and levelup 2

[finnp]

Hey,

I updated the dependencies and change the implementation to pass the tests for the latest abstract-leveldown.

Also: How do you feel about maybe moving this to the level org?
There's a discussion going on at Level/community#14

[ralphtheninja]

Awesome work @finnp ! Maybe also update .travis.yml to use:

• 9
• 8
• 6
• 4

[ralphtheninja]

@finnp Did you have any help of the upgrade guide in abstract-leveldown?

[finnp]

Yes, I had a look at it, that was quite helpful.

[vweevers]

That's awesome to hear! It paid off :)

[vweevers]

Hmm, this would break encodings (for which subleveldown has no tests) as subleveldown expects levelup to handle that but it no longer does. I think wrapping level would work:

var sub = require('subleveldown')
var level = require('level')

var db = level('db')
var subdb = sub(db, 'test', { valueEncoding: 'json' })

But wrapping levelup would not:

var sub = require('subleveldown')
var levelup = require('levelup')
var leveldown = require('leveldown')

var db = levelup(leveldown('db'))
var subdb = sub(db, 'test', { valueEncoding: 'json' })

[ralphtheninja]

Hmm, this would break encodings (for which subleveldown has no tests) as subleveldown expects levelup to handle that but it no longer does. I think wrapping level would work:

Yes, you are right. We should add tests for it to subleveldown as well.

[ralphtheninja]

@vweevers What if we did levelup(encoding(subdown(db, prefix, opts)), opts) here instead?

[vweevers]

Should work, yeah, with one addition: also pass opts to encoding().

[vweevers]

What happens with deferred-leveldown though? Wouldn't every (sub)db be wrapped needlessly?

[ralphtheninja]

Hmm, not following you. That's already the case?

[vweevers]

That's already the case?

It is? OK, then never mind, we can optimize that later if we want

[ralphtheninja]

Aye, so each levelup has its own deferred-leveldown and subleveldown just returns a new levelup so nothing has changed there.

But I agree, it's a bit overhead maybe. Not sure what we can do about it since it's part of levelup.

[ralphtheninja]

@vweevers What if we did levelup(encoding(subdown(db, prefix, opts)), opts) here instead?

Commenting on my own comment 😄

This is exactly what level does, so we could probably do:

return level(subdown(db, prefix, opts), opts)

[ralphtheninja]

Scratch my last comment. I meant using level-packager which wraps with levelup and encoding-down.

[vweevers]

This hurts my brain :)

[ralphtheninja]

This hurts my brain :)

You're not alone :)

[emilbayes]

Just hit this issue today, passing a level@3 to current subleveldown and it tries to double decode json

[mafintosh]

Is this ready for review?

[vweevers]

@mafintosh the missing thing here is the handling of encodings. The suggestion made by @ralphtheninja could work - see #7 (comment). We need unit tests to complete it.

[finnp]

What if we change the API of this to just export a leveldown? Then if someone wants to use encodings they can wrap it in levelup(encoding(..)) themself. As long as the provided levelup doesn't use encodings, it would be fine, right?

And thinking about: Wouldn't it be generally be simpler to just have a module that takes a leveldown instance and returns a sublevel that also passes abstract-leveldown? Or am I missing something? 🤔

[vweevers]

What if we change the API of this to just export a leveldown? Then if someone wants to use encodings they can wrap it in levelup(encoding(..)) themself.

It does already export this, you can require('subleveldown/leveldown') if you want. But that may not even be necessary. Since levelup no longer comes with encodings, I guess it makes sense that when such a db is wrapped with subleveldown it also doesn't come with encodings. Just have to document that fact, recommend usage of level, and release a major.

This should work in userland:

var sub = require('subleveldown')
var levelup = require('levelup')
var encoding = require('encoding-down')
var leveldown = require('leveldown')

var db = levelup(encoding(leveldown('db')))
var subdb = sub(db, 'test', { valueEncoding: 'json' })

I didn't test it though.

Wouldn't it be generally be simpler to just have a module that takes a leveldown instance and returns a sublevel that also passes abstract-leveldown?

Perhaps, but that's something for a new module, to be discussed elsewhere. It doesn't fit subleveldown, which quite conveniently wraps sublevels with levelup.

[ralphtheninja]

Just hit this issue today, passing a level@3 to current subleveldown and it tries to double decode json

@emilbayes Good finding. This should be properly documented in e.g. UPGRADING.md etc.

[ralphtheninja]

@finnp Thanks for your work on this! Will break it apart into smaller changes. Hope you don't mind :)

[ralphtheninja]

v3.0.0-rc1