Update to abstract-leveldown 4 and levelup 2 by finnp · Pull Request #7 · Level/subleveldown

finnp · 2018-02-11T22:33:52Z

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 · 2018-02-12T00:02:37Z

 require('abstract-leveldown/abstract/close-test').close(down, test, testCommon)
 require('abstract-leveldown/abstract/iterator-test').all(down, test, testCommon)
-require('abstract-leveldown/abstract/ranges-test').all(down, test, testCommon)
+require('abstract-leveldown/abstract/iterator-range-test').all(down, test, testCommon)


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

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

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

vweevers · 2018-02-12T22:12:31Z

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 · 2018-02-12T22:15:52Z

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 · 2018-02-13T08:54:43Z

-  }
-
-  return levelup(opts)
+  return levelup(subdown(db, prefix, opts), opts)


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

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

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

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

That's already the case?

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

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.

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

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

This hurts my brain :)

This hurts my brain :)

You're not alone :)

emilbayes · 2018-02-23T09:37:37Z

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

mafintosh · 2018-03-12T20:55:12Z

Is this ready for review?

vweevers · 2018-03-12T22:38:05Z

@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 · 2018-03-12T23:46:31Z

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 · 2018-03-13T01:03:27Z

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 · 2018-05-29T17:43:05Z

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 · 2018-05-29T17:50:53Z

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

ralphtheninja · 2018-06-03T10:14:24Z

v3.0.0-rc1

Update to abstract-leveldown 4 and levelup 2

f177c10

finnp mentioned this pull request Feb 11, 2018

Update to level 2.0 Level/level#84

Closed

ralphtheninja approved these changes Feb 11, 2018

View reviewed changes

Use node 4,6,8,9 on travis

6f63a6e

ralphtheninja reviewed Feb 12, 2018

View reviewed changes

ralphtheninja reviewed Feb 13, 2018

View reviewed changes

vweevers mentioned this pull request Feb 16, 2018

add changelog and upgrade guide Level/level#91

Merged

ralphtheninja mentioned this pull request May 29, 2018

Solve the levelup/level issues #11

Closed

This was referenced May 30, 2018

Current functionality and the future (hoping on support for all use cases) #34

Closed

Update dependencies #39

Merged

ralphtheninja closed this Jun 3, 2018

finnp deleted the update-level branch June 3, 2018 12:03

Uh oh!

Conversation

finnp commented Feb 11, 2018

Uh oh!

ralphtheninja left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vweevers commented Feb 12, 2018

Uh oh!

ralphtheninja commented Feb 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilbayes commented Feb 23, 2018

Uh oh!

mafintosh commented Mar 12, 2018

Uh oh!

vweevers commented Mar 12, 2018

Uh oh!

finnp commented Mar 12, 2018

Uh oh!

vweevers commented Mar 13, 2018

Uh oh!

ralphtheninja commented May 29, 2018

Uh oh!

ralphtheninja commented May 29, 2018

Uh oh!

ralphtheninja commented Jun 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants