Decide on additionalMethods format
vweevers opened this issue · 10 comments
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
}
}
}
One thing the manifest currently can't cover is "static" additional methods like destroy()
and repair()
.
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
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'])
The api looks very nice I think.
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 })
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 })
}
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()
.
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.
@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 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.