Level / abstract-level

Abstract class for a lexicographically sorted key-value database.
MIT License
128 stars 8 forks source link

Cannot use class-based custom encoder #77

Open MeirionHughes opened 12 months ago

MeirionHughes commented 12 months ago

I'm not sure if this would be a bug as its a new code-base afterall; but its certainly a deviation from the leveldown way, where this worked.

class CustomEncoding {
  encode (arg) { return JSON.stringify(arg) }
  decode (arg) { return JSON.parse(arg) };
  constructor () {
    this.format = 'utf8'
    this.type = 'custom'
  }
}

const customEncoding = new CustomEncoding()

const db = testCommon.factory({
  keyEncoding: customEncoding,
  valueEncoding: customEncoding
})

TypeError: The 'encode' property must be a function at new Encoding (D:\Code\abstract-level\node_modules\level-transcoder\lib\encoding.js:28:13) at new UTF8Format (D:\Code\abstract-level\node_modules\level-transcoder\lib\formats.js:75:5) at from (D:\Code\abstract-level\node_modules\level-transcoder\index.js:111:25) at Transcoder.encoding (D:\Code\abstract-level\node_modules\level-transcoder\index.js:66:20) at new AbstractLevel (D:\Code\abstract-level\abstract-level.js:4:432) at new MinimalLevel (D:\Code\abstract-level\test\util.js:123:5) at Object.factory (D:\Code\abstract-level\test\self.js:1078:12) at run (D:\Code\abstract-level\test\encoding-custom-class-test.js:72:27) at Test. (D:\Code\abstract-level\test\encoding-custom-class-test.js:7:7)

vweevers commented 12 months ago

Yeah that's a bug. I'm guessing it's caused by class properties not being enumerable. Or more generally, the assumption that an encoding is a plain object (which is my bad).

vweevers commented 12 months ago

The encoding is cloned as { ...options } here and thus won't include class methods: https://github.com/Level/transcoder/blob/8963603dc4a7c1c599beb85bad3e09788e0235d1/index.js#L111

MeirionHughes commented 12 months ago

yeah I assumed it was cloning issue.

I also notice that in Encoding class, it rebinds the option encode and decode functions 'this'. I have configured custom encoder class that changes the decoding (option to skip certain certain parts of the encoded binary and not decode them), and has its own this context - so I suspect that won't work either.

I can get around this either by wrapping object + closures and/or deriving from abstract class Encoding.

MeirionHughes commented 12 months ago
function safeOptions(base, name) {
  const options = { name: name || base.name, format: base.format };
  options.encode = base.encode ? function (...args) { return base.encode(...args) } : null;
  options.decode = base.decode ? function (...args) { return base.decode(...args) } : null;
  return options;
}

switch (detectFormat(options)) {
  case 'view': return new ViewFormat(safeOptions(options, name))
  case 'utf8': return new UTF8Format(safeOptions(options, name))
  case 'buffer': return new BufferFormat(safeOptions(options, name))
  default: {
    throw new TypeError("Format must be one of 'buffer', 'view', 'utf8'")
  }
}

lets the test (with a class) pass; not sure if you'd rather do it differently

vweevers commented 12 months ago

I'd be okay with a short-term solution like that (and to later refactor it). I don't have capacity atm to think about alternatives.