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

Drop support of key types other than string and Buffer #191

Merged
merged 3 commits into from
Aug 17, 2019

Conversation

vweevers
Copy link
Member

@vweevers vweevers commented Aug 14, 2019

@vweevers vweevers added the semver-major Changes that break backward compatibility label Aug 14, 2019
@vweevers
Copy link
Member Author

Does this warrant a prerelease? That hasn't proven itself useful in the past (on levelup, leveldown) mainly because there wasn't any feedback on prereleases.

@vweevers vweevers mentioned this pull request Aug 14, 2019
@ralphtheninja
Copy link
Member

Does this warrant a prerelease? That hasn't proven itself useful in the past (on levelup, leveldown) mainly because there wasn't any feedback on prereleases.

I think we can skip that, because no feedback and also yolo.

@MeirionHughes
Copy link
Member

MeirionHughes commented Aug 14, 2019

with "memdown": "git://github.com/Level/memdown.git#drop-custom-key-types", the failing test in https://github.com/Level/subleveldown/tree/tests/buffer-race-issue passes. I've hooked this up with my store db, switching from charewise to bytewise and its passing my unit tests.

@vweevers
Copy link
Member Author

The race issues in level-ttl make it hard to tell if it is now compatible with memdown (less tests are failing, basically) so I also added a small test here, of mixed use of string and Buffer keys.

TLDR; everything looks good.

@vweevers
Copy link
Member Author

One open question: this PR fixes keys, but similar issues exist on values. For example, assume we have a level-mem db with utf8 value encoding, then await db.put('key', Buffer.from('foo')); await db.get('key') yields a Buffer, but with leveldown we'd get a string.

Should we go all the way, and also drop custom value types?

@vweevers
Copy link
Member Author

Should we go all the way, and also drop custom value types?

I say yes, because it's another incompatibility that isn't easy to solve otherwise. And it's common to swap keys and values (e.g. for indexing) so they should behave largely the same (I wish I realized this sooner).

Will open a second PR.

@vweevers vweevers merged commit b442455 into master Aug 17, 2019
@vweevers vweevers deleted the drop-custom-key-types branch August 17, 2019 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major Changes that break backward compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants