Carefully consider behavior of nullish range options

[vweevers]

Though the logic in f267647 felt "right", I'm afraid it could break the ecosystem. It could be incompatible with popular tools like ltgt. For example, ltgt.lowerBound({}) returns undefined.

If you're wondering why this doesn't cause problems for encoding-down, it has special logic for nullish. It doesn't touch them in the serialization step or in the encoding step (of builtin encodings). This avoids { gt: undefined } turning into { gt: 'undefined' }.

I'm leaning towards reverting f267647, perhaps even the abstract-leveldown behavior (so that nullish means "not defined" again everywhere), but this needs some tests and thinking. If possible, I want to avoid needing special logic for nullish in every abstract-leveldown implementation. Because really, the only odd duck is encoding-down.

Perhaps, if we revert the abstract-leveldown behavior, a new abstract _isValidTarget() method (bikeshed name) could allow encoding-down to enable nullish range options:

// Any range option for which _isValidTarget returns false, will be removed 
// from the options object by abstract-leveldown
AbstractLevelDOWN.prototype._isValidTarget = function (target) {
  return target != null
}

EncodingDOWN.prototype._isValidTarget = function (target) {
  return true
}

[vweevers]

I'm leaning towards reverting f267647, perhaps even the abstract-leveldown behavior (so that nullish means "not defined" again everywhere).

That was fear talking!

First, find out how and where ltgt and also level-option-wrap are used. And if there is a situation where {} turns into { gt: undefined, gte: undefined, .. }. If not, we're good.

[vweevers]

And if there is a situation where {} turns into { gt: undefined, gte: undefined, .. }

Yes. Run:

var wrap = require('level-option-wrap')

// no gt property
var input = {}

var output = wrap(input, {
  gt(x) { return x }
})

if ('gt' in output) {
  console.log('we have a problem')
}

Still, I struggle to find a case where this is actually a problem. For example, subleveldown uses level-option-wrap, but it also replaces falsy range options with empty strings, so no problem there.

And the above behavior is arguably a bug in level-option-wrap. It should not add properties that don't exist in the input object. Edit: nah, then subleveldown wouldn't work! The functions passed to level-option-wrap also serve to define defaults.

[vweevers]

To recap: historically, nullish range options have meant "not defined" in most of the ecosystem, but because it's most and not all, it required a large number of hacks, many undocumented and without integration tests, making it hard to predict behavior, let alone how a change to that behavior will ripple through the ecosystem.

In abstract-leveldown v6, nullish range options no longer mean "not defined". Thanks to this, implementations like encoding-down can give their own meaning to nullish range options. I'm positive we made the right call there. Though it is scary to change a common behavior in the ecosystem, or rather, to demand a change, I think it would be a mistake to reintroduce "nullish means not defined" in leveldown and (by inspiration) every other implementation. The added code complexity counts as yet another hack in my book.

Yes, there is risk of ecosystem breakage, but we have to start somewhere, with a documented, simplest possible behavior, which we did for abstract-leveldown and now leveldown.