Remove stringification of keys and values

[juliangruber]

levelup handles massaging keys and values through its encoding layer already. However, if the user passed encoding: 'id', abstract-leveldown would still convert everything to a string or buffer. For those encodings, like level.js and postgres-down that do support more of the native JS values, this gives them the chance of receiving the raw values and encoding them themselves.

It's a breaking change and so if merged will need a 3.0.0 release.

[joeybaker]

I never leave a drive-by 👍, but @juliangruber asked for 'em, so: +1 from me!

[juliangruber]

thanks @joeybaker ❤️

[ralphtheninja]

+1

[deanlandolt]

Huge 👍

[rvagg]

How could we best communicate this change to downstream users? It's a pretty big one and could open up some problems that won't necessarily be picked up by test suites.

[deanlandolt]

Perhaps this could be opt-in? A prototype key an implementation has to set to remove this behavior?

ISTM this would would also help address #83, #84 as well (and a number of other issues across a whole slew of repos, I'm sure).

[juliangruber]

or

function DB(){
  AbstractLevelDOWN.call(this, name, {
    toBuffer: false
  })
}

[rvagg]

That'd actually be a great first step regardless, make it opt-in for now, switch to opt-out and maybe remove in the future.

[juliangruber]

check this out

[deanlandolt]

LGTM 👍

But why not this._db._toBuffer in AbstractChainedBatch?

An alternative (perhaps a bit more flexible) design just came to mind: add a _serialize function to AbstractLevelDOWN like this:

AbstractLevelDOWN.prototype._serialize = function (input) {
  return this._isBuffer(input)) ? input : String(input)
}

Then on AbstractChainedBatch you can just add another _serialize function that falls through to this._db._serialize, but allows you to customize the behavior per chained batch. This would represent the last step before data gets pushed down into the store. This would also allow you to, e.g. force values into typed arrays, or whatever a given backing store prefers, w/o having to completely reimplement the rest of the write logic.

Does this sound reasonable?

[juliangruber]

this sounds mega perfect! i've thought about this for a little and it so makes sense. i'll work on it

[juliangruber]

actually, it needs to be slightly more complicated, because serialization depends on whether it's a key or a value. i'll create _serializeKey and _serializeValue

[juliangruber]

ok how does this look?

[deanlandolt]

@juliangruber: that's beautiful, man! 👍

[juliangruber]

added a couple missing tests and fixed the bug magnus discovered. mergeable as a minor?

[ralphtheninja]

mergeable as a minor?

+1

[juliangruber]

thanks @ralphtheninja and @deanlandolt! @rvagg can i get a +1 from you as well?

[deanlandolt]

👍

[juliangruber]

2.5.0 it is!

[nolanlawson]

So it looks like this was indeed a breaking change, because AFAICT from git-bisecting, this commit broke memdown: Level/memdown#55

[nolanlawson]

BTW maybe we should start using something like dont-break in this project, because I feel like we run into the situation a lot where somebody merges something and then it randomly breaks FooDOWN somewhere in the ecosystem.

[ralphtheninja]

BTW maybe we should start using something like dont-break in this project, because I feel like we run into the situation a lot where somebody merges something and then it randomly breaks FooDOWN somewhere in the ecosystem.

+1

[juliangruber]

you're right that this actually is a breaking change. Before, null and undefined would be stored as empty string values, now they get stored as "null" and "undefined". Those exact tests are failing: https://github.com/Level/abstract-leveldown/blob/master/abstract/put-get-del-test.js#L140-L141.

I'm working on a pull request to revert this breaking change.

[juliangruber]

Fixed in 7f514f4, committed straight to master and released as 2.6.1. The memdown test suite passes with this version :)

[nolanlawson]

Awesome, thank you!! :) So just to check - this changes the on-disk representation of data? I.e. if I created data with <2.5.0, if I then open it in 2.6.0 the data may be changed/corrupted?

[juliangruber]

it changes how data nullish values are stored in 2.6.0., 2.6.1 however will behave the same as 2.5.0

[nolanlawson]

Commented on this in Level/memdown#57 (comment), but there's something basic I'm not understanding: if I write data in 2.4.1, then read that same data in 2.6.1, is the data changed/corrupted? If so, then 2.5.0 should have been a major version bump, and in addition it would need to offer some guidelines on how to migrate userdata over (or determine if a migration is necessary).

Yes this only applies to nullish data, and yes memdown isn't affected, but this project has a lot of dependents so it's worth communicating downstream if package authors need to figure out a migration path.

[juliangruber]

only 2.6.0 is the faulty version. 2.6.1 and 2.5.0 have (or should have) full compatibility with anything done with 2.4.1.

[nolanlawson]

OK, I misunderstood. Thanks for clarifying!