Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot use class-based custom encoder #77

Open
MeirionHughes opened this issue Nov 13, 2023 · 5 comments
Open

Cannot use class-based custom encoder #77

MeirionHughes opened this issue Nov 13, 2023 · 5 comments
Labels
bug Something isn't working pull request welcome A pull request is welcome

Comments

@MeirionHughes
Copy link
Member

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
Copy link
Member

vweevers commented Nov 13, 2023

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 vweevers added the bug Something isn't working label Nov 13, 2023
@vweevers
Copy link
Member

vweevers commented Nov 13, 2023

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
Copy link
Member Author

MeirionHughes commented Nov 13, 2023

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
Copy link
Member Author

MeirionHughes commented Nov 13, 2023

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
Copy link
Member

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.

@vweevers vweevers added the pull request welcome A pull request is welcome label Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pull request welcome A pull request is welcome
Projects
None yet
Development

No branches or pull requests

2 participants