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

Update multileveldown and level-party #71

Closed
MattMorgis opened this issue Jun 4, 2019 · 24 comments
Closed

Update multileveldown and level-party #71

MattMorgis opened this issue Jun 4, 2019 · 24 comments

Comments

@MattMorgis
Copy link

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 [email protected] and [email protected] (from [email protected].

It seems a refactor to leveldown@5x would require a refactor to abstract-leveldown@v6 and [email protected]+, 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
Copy link
Member

vweevers commented Jun 4, 2019

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

MattMorgis commented Jun 4, 2019

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" ([email protected] and [email protected]), 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" ([email protected] and [email protected]), 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
Copy link
Member

vweevers commented Jun 4, 2019

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

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 [email protected]+, 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
Copy link
Member

vweevers commented Jun 4, 2019

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

vweevers commented Jun 4, 2019

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

@MattMorgis
Copy link
Author

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

@vweevers
Copy link
Member

vweevers commented Jun 4, 2019

@vweevers vweevers changed the title Possible to add Node.js 12 support to v4? Update multileveldown and level-party Jun 4, 2019
@vweevers vweevers transferred this issue from Level/leveldown Jun 4, 2019
@MattMorgis
Copy link
Author

MattMorgis commented Jun 4, 2019

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

vweevers commented Jun 4, 2019

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

@binarykitchen
Copy link

binarykitchen commented Sep 2, 2019

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

vweevers commented Sep 3, 2019

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

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

@vweevers
Copy link
Member

vweevers commented Sep 5, 2019

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

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

@vweevers
Copy link
Member

vweevers commented Sep 5, 2019

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

@vweevers vweevers self-assigned this Sep 7, 2019
@vweevers
Copy link
Member

vweevers commented Sep 7, 2019

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

Moving on to level-party.

@vweevers
Copy link
Member

vweevers commented Sep 7, 2019

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

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

@vweevers
Copy link
Member

vweevers commented Sep 7, 2019

  • subleveldown on level-party works
  • subleveldown on multileveldown client works (i.e. for client-side sublevels)
  • multileveldown server on subleveldown works (i.e. to expose only a sublevel to client)

@vweevers
Copy link
Member

vweevers commented Sep 8, 2019

Opened Level/multileveldown#15 and Level/party#20.

@binarykitchen
Copy link

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

@vweevers vweevers removed their assignment Sep 15, 2019
@vweevers
Copy link
Member

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

@binarykitchen
Copy link

Cool, I'm getting excited ...

@vweevers
Copy link
Member

vweevers commented Dec 8, 2019

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

@vweevers vweevers closed this as completed Dec 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants