Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Treat value null/undefined as a del? #191

Closed
jcw opened this Issue · 8 comments

4 participants

@jcw

Would it be possible to treat a put with a null/undefined value as a del?

This could simplify the API in many places, by having the same structure for the put and del cases. Given that both use the same number of values, you can do things like:

somecall({key: 'abc', value:123, type: 'put'}, ...)
  becomes:  somecall('abc', 123, ...)
somecall({key: 'abc', type: 'del'}, ...)
  becomes:  somecall('abc', null, ...)

Also for callbacks. It may be possible to fold this in compatibly, but callback changes probably will need their own calls.

If there is any interest, and if this looks worthwhile, I'll try to monkey-patch this change.

Or is there any reason why this would not be a good idea?

@mcollina
Owner

I think it is a bad idea because empty-valued data structures are possible in this situation, while with your change they will be not possible.
However, there are plenty of use cases for you fix, so I think you should build a wrapper for LevelUp to implement this.

This will also give us the possibility to use and test this before merging your change. If we see adoption of your change, we might re-evaluate.

This are just my two cents, let's see what the others think!

@jcw

Couldn't the empty string be used to represent empty values?

Although with JSON encoding, it would not be zero bytes, and there would also be [] and {} to represent "emptiness".

@mcollina
Owner

Of course yes, but it's a disruptive API change, you want to merge put and del.

Write a module and let everybody test your idea :D.

@jcw

Ok. What would be your suggestion: fork and mod node-levelup, or create a wrapper which does modified calls and intercepts all the callbacks?

A change like this does seem disruptive. Not sure just how it can fit in with all the extensions already out there...

@rvagg
Owner

Put this in a module and publish it to npm, then you'll have a simple wrapper that'll fix levelup to do what you want and you can share it with others, I'm sure you're not alone in wanting this.

function nulldel (db) {
  var put = db.put
  db.put = function (key, value) {
    if (value === undefined || value === null) {
      Array.prototype.splice.call(arguments, 1, 1)
      return db.del.apply(db, arguments)
    }
    put.apply(db, arguments)
  }
  return db
}

I think it changes the explicit semantics of put operation at bit too much for me, I can understand why it might be nice to have something like this but there's just a little too much indirection going on in there.

It is true that you just can't put undefined in to a put() as it is now, you'll get an error, null is valid if you're using JSON encoding. But now that we have custom encoders & decoders undefined could also be mapped to something by an encoder. Best be simple and explicit at the lowest layer to allow creativity on top.

@dominictarr
Owner

Yeah, this is a breaking change for a very minor benefit.
We discussed this change some months back, and we didn't decide to change it,
as @rvagg felt it was too implicit.

This is just one of those things we have to live with now, it's far too much work to change
little things like this, when there are so many modules that use levelup.
The changes we need now are about performance or correctness, or maybe blessing common patterns.

@jcw

Thanks @rvagg - and done, see https://npmjs.org/package/level-nulldel.

@dominictarr - understood, although in the long term I wouldn't consider this benefit minor. Then again, I'm starting fresh with levelup, so for me historical decisions don't weigh as heavily. The workaround proposed above is great, so I'm fine with it either way.

@jcw

On the event side, I've added the following line to my code:

db.on('del', function (key) { db.emit('put', key) });

This may break some extensions though, since now deletion will fire both events. Will see how it goes.

@rvagg rvagg closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.