search

Level / community

Discussion, support and common information for projects in the community.
MIT License
42 stars 15 forks source link

Update multileveldown and level-party #71

Closed MattMorgis closed 4 years ago

MattMorgis commented 5 years ago

Hi,

I understand leveldown@v5 is the minimum version for Node.js 12 support.

Is it feasible to add Node.js 12 support to v4? I'd be happy to help out, but curious if it's a lost cause from the start.

I ask because I am working on updating level-party. I was able to get it to run on Node.js 10 and 11 by having it use a combination of levelup@1.x and leveldown@4.x (from leveldown@1.x.

It seems a refactor to leveldown@5x would require a refactor to abstract-leveldown@v6 and levelup@2.x+, which has some changes to encodings. If it's possible to add Node.js 12 support to v4, I could avoid the big reactor for now.

vweevers commented 5 years ago

I ask because I am working on updating level-party

What doesn't work when you combine level-party with leveldown@5?

I prefer fixing that over maintaining two release lines of leveldown.

MattMorgis commented 5 years ago

I prefer fixing that over maintaining two release lines of leveldown.

Totally understand. I am new to all of these level modules, and am trying to be a good open source citizen and update level-party as it failed to install when we upgraded to Node.js 10. I will take any guidance on how to upgrade it properly.

I have a fork of level-party here.

If you change this to "1.8.0" (levelup@1.x and levelup@2.x), it will work on Node.js 10 and 11, but won't install on Node.js 12. https://github.com/MattMorgis/level-party/blob/node-10-support/package.json#L7

If you change it to "1.8.1" (levelup@1.x and leveldown@5.x), it installs on Node.js 12, but npm test will fail across all versions. https://github.com/MattMorgis/level-party/blob/node-10-support/package.json#L7

The culprit is this test, specifically this line not getting called. I've gone down many rabbit holes trying to figure out why, but none have seemed to lead to the answer so far. https://github.com/MattMorgis/level-party/blob/master/test/bytewise.js#L31

vweevers commented 5 years ago

I think the first step is to update https://github.com/mafintosh/multileveldown. Then we might need not even need API changes in level-party, which is just a thin wrapper around multileveldown.

MattMorgis commented 5 years ago

I've been down that rabbit hole. I have my own fork here, which was from this fork.

It seems when upgrading multileveldown to use levelup@2.x+, and bringing encoding-down into the mix breaks a lot there and which bubbles up into level-party

I can submit something that reproduces it, but the main tests that broke seemed to be almost there. It would want int 64, but instead would get the Buffer of that data or string "64".

vweevers commented 5 years ago

Yeah, I had a look at multileveldown just now. With a few tweaks, gets and puts and dels are working, read streams are not. I can take a closer look at the end of the week.

I'm afraid that rabbit hole (which I agree it is!) is the only proper way to fix this.

/cc @mafintosh Are you still (interested in) maintaining multileveldown? If not, we could consider moving it. But maybe not to Level at first, there's enough work here.

vweevers commented 5 years ago

OK, got all multileveldown tests to pass. But to make it work @ralphtheninja I'm afraid we're gonna have to revert this revert: https://github.com/Level/codec/pull/23 (I will attempt to explain later)

MattMorgis commented 5 years ago

Interesting! Do you have a version pushed up to GitHub? I'd love to try it with level-party.

vweevers commented 5 years ago

https://github.com/vweevers/multileveldown/tree/wip-upgrade

MattMorgis commented 5 years ago

Tried it out here: https://github.com/MattMorgis/level-party/tree/node-10-support

Still getting the same error I mentioned above in level-party. It seems to get back the raw Buffer data back:

not ok 2 should be equivalent
  ---
    operator: deepEqual
    expected: [ 'a' ]
    actual:   <Buffer a0 70 61 00 00>
    at: ReadStream.<anonymous> (/level-party/test/bytewise.js:27:15)
  ...
not ok 3 should be equivalent
  ---
    operator: deepEqual
    expected: 22319
    actual:   { data: [ 50, 50, 51, 49, 57 ], type: 'Buffer' }
    at: ReadStream.<anonymous> (/level-party/test/bytewise.js:28:15)
vweevers commented 5 years ago

It might be helpful to copy (and adapt) that test to multileveldown. Then fix it there without level-party muddying the waters.

binarykitchen commented 5 years ago

Just scanned your conversations and salute your hard work to update both libraries. Really can't wait to upgrade these because Node.js v8 nears end-of-life this December, so ...

That said, what's the current state and what are we waiting for?

vweevers commented 5 years ago

Recent changes in memdown will make it easier to upgrade and test these. There's a few related releases to get out the door.

I'll move this task up in the project board.

binarykitchen commented 5 years ago

@vweevers can I help with that somehow? To speeden up things?

vweevers commented 5 years ago

I don't remember all the details, but you could take this commit and attend the TODO comments: https://github.com/vweevers/multileveldown/commit/88e284d395f6d80403ad53e8a04bd6b22ef59dc3. I'll add some GitHub comments to that commit as well, for context.

binarykitchen commented 5 years ago

Sorry @vweevers I am more into level-party, want to make it work for Node.js v10

vweevers commented 5 years ago

level-party depends on multileveldown; getting multileveldown up to par is the first step.

vweevers commented 5 years ago

multileveldown is mostly complete: https://github.com/vweevers/multileveldown/commits/upgrade

Moving on to level-party.

vweevers commented 5 years ago

Tests are passing: https://github.com/vweevers/level-party/commits/upgrade.

More tweaks are likely needed to also make it work with subleveldown.

vweevers commented 5 years ago
vweevers commented 4 years ago

Opened https://github.com/mafintosh/multileveldown/pull/15 and https://github.com/substack/level-party/pull/20.

binarykitchen commented 4 years ago

@vweevers thanks so much! hope they will pass review, merged and published soon.

vweevers commented 4 years ago

multileveldown has moved to Level and is ready for release, just waiting for npm ownership.

binarykitchen commented 4 years ago

Cool, I'm getting excited ...

vweevers commented 4 years ago

level-party@4 is out (and has moved to Level as well).