Use undefined instead of error for non-existing records

This is a request for discussion about the semantics of db.get() when a key doesn't exist. Currently, an error with a non-enumerable type key is provided for missing keys:

db.get('does-not-exist', function (err, value) {
  console.log('err=', err);
  console.log('err.type=', err.type);
})

produces this output:

err= {}
err.type= NotFoundError

The error is not easy to inspect at a glance (a separate issue) and if you want to check for non-existence you've got to remember that the err.type for non-existence is NotFoundError.

Would it make more sense to just use undefined for the value and not go through the err parameter? undefined is already an invalid value to use for storage so it wouldn't conflict with anything unless there are some edge cases for custom valueEncodings I'm not aware of.

[juliangruber]

Level/levelup#223 (comment) :)

[rvagg]

As @juliangruber pointed out, undefined and null will soon be perfectly acceptable values to write in levelup, they'll actually be stored (as zero-length slices) and even show up in a ReadStream. That complicates this discussion somewhat.

I think most people would agree that NotFoundError feels like one of the most awkward places of the current levelup API and it'd be nice to have something a bit cleaner, I'm just not sure this is it. To me the current API reminds me a little of the awkwardness of using fs.stat() so it's not entirely out of place in Node.

Something worth pointing out is that we did add a notFound property to NotFoundError (as well as a 404) so you can actually do something like this:

db.get('does-not-exist', function (err, value) {
  if (err && err.notFound) {
    // .. handle does-not-exist
  } else if (err) {
    return callback(err)
  }

  // .. handle exists
})

[juliangruber]

aaaah yeah I confused empty slices with non existing keys.

talking about readstream, wouldn't valuestreams end once you push a null?

[kemitchell]

@juliangruber: Found this issue after rehashing the loathsome error.notFound boilerplate one too many times, then found your comment on null.

Wrote a quick test showing inconsistent behavior between leveldown and memdown for null and undefined values emitted by .createValueStream() return values. Here is the test. Shell session in the README.

Long story short:

1. With leveldown, null and undefined come out as "".
2. With memdown, null and undefined come out as "null" and "undefined".

[kemitchell]

Stepping back to think about the error problem again, I suppose there are three approaches to eeking out a side channel for saying "there isn't a value" that haven't been mentioned yet:

1. Pick another JavaScript value that can't be a levelup value and anoint it the new nothing value. NaN? This does not feel like a good idea.
2. Contextualize all values, say by wrapping like {stored: null}. Key-value pairs become {key:'x', value: { stored: null } } or { key: 'x', value: { } }. Have value streams emit {} and .get yield {} rather than error with .notFound.
3. Export an additional object, like levelup.NO_VALUE, and rely on value === levelup.NO_VALUE for the check.

The second doesn't seem all that bad. I suppose it boils down to the relative inconveniences of an extra .stored (or whatever) accessor, on the one hand, and not being able to play nice with the universe of packages and functions that rely on error-first as a channel exclusively for it-done-broke-caliber problems, on the other.

[vweevers]

Using undefined as a "not found" signal is now viable again, because null and undefined are no longer valid values (Level/abstract-leveldown#120). Let's give that a few more months in the wild (although I'd say the decision to drop null and undefined is final) before possibly re-evaluating this.