Decide on additionalMethods format

[vweevers]

Initially we can do:

{
  additionalMethods: {
    approximateSize: true
  }
}

Which is enough to start using that in deferred-leveldown (Level/deferred-leveldown#35) and level-packager levelup. @ralphtheninja WDYT?

Later we can add function signature info. I propose to stick with boolean properties, so that we can describe multiple signatures (e.g. callbacks and promises):

{
  additionalMethods: {
    approximateSize: {
      sync: false,
      callback: true,
      promise: false
    }
  }
}

With that in place, a db wrapper could do things like:

wrapper.prototype.approximateSize = function (..) {
  if (this.db.supports.additionalMethods.approximateSize.promise) {
    return db.approximateSize(..)
  } else {
    // Wrap in promise
  }
}

Even later, we can add a return type to the manifest. Mainly for streams. Not sure what that should look like. Perhaps:

{
  additionalMethods: {
    createWriteStream: {
      sync: false,
      writable: true
    }
  }
}

[vweevers]

One thing the manifest currently can't cover is "static" additional methods like destroy() and repair().

[vweevers]

To use the manifest in encoding-down to automatically encode arguments of approximateSize() etc, we need a way to describe arguments. I made a draft PR of what that might look like: Level/encoding-down#93

[vweevers]

Combined with level-compose, a plugin pattern starts to emerge:

function myIndexPlugin (db) {
  db.supports.additionalMethods.createIndex = {
    args: [{ type: 'key' }]
  }

  db.createIndex = function (prefix) {
    // prefix is encoded
  }

  return db
}

const factory = compose()
  .use(leveldown)
  .use(myIndexPlugin)
  .use(encode, { keyEncoding: charwise })
  .use(levelup)

const db = factory()

db.createIndex(['my_indexes', 'foo'])

[ralphtheninja]

The api looks very nice I think.

[vweevers]

A more complex example, where encoding-down has to decode output of a hypothetical getAll() method. Trying out a format compatible with superstruct (compact, but would require parsing). We'll at some point hit the same problem that the typescript folks had, with return types that depend on options:

function getAllPlugin (db) {
  db.supports.additionalMethods.getAll = {
    args: ['options?', 'callback'],
    yields: ['entry'] // TODO: type depends on options
  }

  db.getAll = function (options, callback) {
    const it = db.iterator(options)

    concat(it, function (err, entries) {
      if (err) return callback(err)
      
      if (options.keys === false) {
        entries = entries.map(e => e.value)
      } else if (options.values === false) {
        entries = entries.map(e => e.key)
      }

      callback(null, entries)
    })
  }

  return db
}

const factory = compose()
  .use(leveldown)
  .use(getAllPlugin)
  .use(encode, { valueEncoding: 'json' })
  .use(levelup)

const db = factory()
const entries = await db.getAll({ lt: 18 })
const values = await db.getAll({ lt: 18, keys: false })

[vweevers]

Perhaps we should simply stop using that pattern ({ keys: false }), at least in public APIs. On read streams for example, we could hide it like so:

// Yields entries, values or keys
db._internalCreateReadStream = function (options, ..) {
  if (options.keys === false) // ..
}

// Always yields entries
db.createReadStream = function (..) {
  return this._internalCreateReadStream(..)
}

// Always yields values
db.createValueStream = function (..) {
  return this._internalCreateReadStream({ keys: false })
}

[vweevers]

Although if you swap the layers:

.use(encode, { valueEncoding: 'json' })
.use(getAllPlugin)

Then the problem disappears, because the db.iterator() used by getAll() already does the encoding & decoding. And the manifest doesn't have to describe the signature of getAll().

[vweevers]

If we want dynamic return types like described above, then the manifest would also have to be dynamic:

{
  args: ['options?', 'callback'],
  yields (options) {
    if (options.keys === false) return ['value']
    if (options.values === false) return ['key']
    return ['entry']
  }
}

Which is not impossible, but...:

Click to expand encoding-down example

function EncodingDown (db, opts) {
  AbstractLevelDOWN.call(this, db.supports)

  // ..

  Object.keys(this.supports.additionalMethods).forEach((m) => {
    var signature = this.supports.additionalMethods[m]

    this[m] = function () {
      var i = Math.min(arguments.length, signature.args.length)
      var args = []
      var opts
      var callback

      while (i--) {
        if (signature.args[i] === 'callback') {
          callback = arguments[i]
        } else if (signature.args[i] === 'options' && !opts && isOptions(arguments[i])) {
          opts = arguments[i]
          args.unshift(this.codec.encodeLtgt(opts))
        } else if (signature.args[i] === 'key') {
          args.unshift(this.codec.encodeKey(arguments[i], opts))
        } else {
          args.unshift(arguments[i])
        }
      }

      const resultType = signature.yields(opts)
      const decode = (result, type) => {
        if (type === 'key') {
          return this.codec.decodeKey(result, opts)
        } else if (type === 'value') {
          return this.codec.decodeValue(result, opts)
        } else if (type === 'entry') {
          return {
            key: this.codec.decodeKey(result.key, opts),
            value: this.codec.decodeValue(result.value, opts)
          }
        } else {
          return result
        }
      }

      args.push(function (err, result) {
        if (err) return callback(err)

        if (Array.isArray(resultType)) {
          result = result.map((r, i) => decode(r, resultType[i]))
        } else {
          result = decode(result, resultType)
        }

        callback(null, result)
      })

      return this.db[m].apply(this.db, args)
    }
  })
}

I don't want to go there. For situations like this we should instead write some abstraction like hooks or perhaps _deserialize* (symmetric with _serialize*) to process keys, values or entries.

[vweevers]

@ralphtheninja TLDR, I don't want args or return types in the manifest for now. And maybe never.

I'll update Level/encoding-down#93 to simply do DB.prototype.approximateSize = .. like before.

[ralphtheninja]

@ralphtheninja TLDR, I don't want args or return types in the manifest for now. And maybe never.

It's a fine line when to stop engineering stuff.