Skip to content
This repository has been archived by the owner on Dec 6, 2018. It is now read-only.

expose a leveldown instance #81

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Raynos
Copy link
Contributor

@Raynos Raynos commented Dec 22, 2014

In the same vain as #77

A levelup instance exposes a db property that is a leveldown
instance. This is documented in the levelup README.

Here we expose the leveldown instance through nut and also
work around the fact that levelup uses a DeferredLevelDown
instance and later mutates itself to set a real leveldown instance
to the db field.

cc @dominictarr

@dominictarr
Copy link
Owner

so this s/exposes/leaks/ the global leveldown, it doesn't expose the prefix managed handle?
What do you need to use this for?

@Raynos
Copy link
Contributor Author

Raynos commented Dec 22, 2014

@dominictarr

You make a very good point.

I'm using level-session which uses level-ttl which uses level-spaces

level-spaces uses a module called level-updown ( https://github.com/rvagg/level-spaces/blob/master/level-spaces.js#L2 ).

This module is a technique @rvagg uses to wrap a levelup with hooks similar to techniques level-sublevel uses.

The way level-updown works is it re-implements the leveldown interface on top of levelup so that you can you do var db = levelup({ db: function () { return udb(originalDb) })

For level-updown to work the originalDb must have a .db field with an iterator() method ( https://github.com/rvagg/level-updown/blob/master/level-updown.js#L33-L35 ).

It looks like either level-sublevel will have to prefix the leveldown instance or level-sublevel and level-spaces cannot live together.

@dominictarr
Copy link
Owner

this sounds like a crazy rube goldburg machine. Are you using this inside just one sublevel, or are you using on the top most sublevel?

If I did merge this, what would the testcase that ensures we have the right behavior look like?

@Raynos
Copy link
Contributor Author

Raynos commented Dec 22, 2014

@dominictarr I think this PR will break. I need to prefix the leveldown instance.

I think sublevel and spaces inception is going to end in crazy town.

@dominictarr
Copy link
Owner

yes. so my hunch is that level-ttl is too complicated and that it could be simplified. It was written very early in the level era, and we know a lot more now.

@Raynos
Copy link
Contributor Author

Raynos commented Dec 23, 2014

@dominictarr

I could fix level-ttl. There is larger problem though which is level-spaces and level-sublevel solve a similar problem of "implementing multiple tables" and are not compatible with each other.

It would be good to have a long term solution for a "subsection of a leveldb with a levelup interface". Especially if multiple of those can be used together without breaking around the edges.

cc @rvagg @dominictarr

@dominictarr
Copy link
Owner

it seems @rvagg made level-spaces because he was annoyed at some of the complexities of sublevel (especially since v6 when level-ttl stopped working with level-sublevel) it's not the first time that someone has tried to rewrite level-sublevel, This is strong evidence it's a good idea, but that it's not perfected yet.

The pattern I ended up with on v6 is that there is a core part (nut.js) which is like the leveldown, and then a wrapper (shell.js) which implements a particular prefix. this exposes the levelup methods, and prepares a batch call on the inner nut. @rvagg has done something like this too, except using levelupdown to get another leveldown... this is also a very interesting idea.

@rvagg
Copy link
Contributor

rvagg commented Dec 23, 2014

Re the initial issue--this comes back to my criticism of the sublevel rewrite--it pretends to expose a new LevelUP instance but it's too different and breakage happens whenever someone tries to use it as if it's a real LevelUP instance (like a missing .db property, but this is just one example of a number).

I agree that level-ttl has got too complicated and could be simplified -- @Raynos I'm happy for you to be more heavy-handed on that front.

I've been seeing sublevel (et. al.) as plumbing components of the level ecosystem so that libs like level-ttl could use them to do internal account-keeping in an easy way, but the crazy dependency webs and conflicts we keep on running in to suggest we're still doing it wrong and need to back up and make things more independent. The monkey-patching of the LevelUP object is probably the wrong approach because it requires matching dependencies everywhere, instead we should make sure that the abstractions are doing their job in isolation.

@dominictarr
Copy link
Owner

@rvagg you are probably correct. What are the other things you consider missing on sublevel, apart from the missing .db property? (we recently fixed isOpen, isClosed and close, btw)

@rvagg
Copy link
Contributor

rvagg commented Dec 24, 2014

All of the above plus the additional edge cases you've run in to (my main problem, more than the missing features) and are going to keep running in to as you try and make it exactly compatible with levelup. I consider Level/levelup#273 to be a criticism of the complexity and un-reusability of levelup code than pointing to a real need for an abstract test suite. The fact that you're not (able to?) do a full re-use of levelup code is the problem here because you're simply reimplementing it and will have to keep in lock-step with any changes that happen there which is just going to cause rolling problems into the future as we have multiple versions of these levelup-ish interfaces floating around.

What I'd really like to see you do with sublevel is pull in more levelup code so that you're not reimplementing.

@dweinstein
Copy link
Contributor

I guess this conversation is related to #84 as well?

@dominictarr
Copy link
Owner

@rvagg that could probably be done. if levelup had prehooks then sublevel could use that... then level-sublevel would just be a particular configuration of levelup.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants