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 to use level 8 #199

Merged
merged 21 commits into from Apr 24, 2024
Merged

Update to use level 8 #199

merged 21 commits into from Apr 24, 2024

Conversation

finwo
Copy link
Member

@finwo finwo commented Apr 7, 2024

Includes:

Extras:

  • Adds a polyfill for levelup.createValueStream (and related methods), as that has been ditched with level 8 using it's default classic-level export

@vweevers
Copy link

vweevers commented Apr 7, 2024

FYI for the streams, see https://github.com/Level/read-stream

@finwo
Copy link
Member Author

finwo commented Apr 7, 2024

FYI for the streams, see https://github.com/Level/read-stream

Awesome, I'll try to implement that tomorrow instead of the custom workaround

@finwo
Copy link
Member Author

finwo commented Apr 8, 2024

Just tried using level-read-stream, but I ran into the following error:

     Uncaught TypeError: db[method] is not a function
      at new LevelReadStream (node_modules/level-read-stream/index.js:17:33)
      at new ValueStream (node_modules/level-read-stream/index.js:80:5)
      at Object.getStream (lib/levelgraph.js:60:20)
      at JoinStream.transform [as _transform] (lib/joinstream.js:95:30)
      at Transform._write (node_modules/readable-stream/lib/internal/streams/transform.js:153:8)
      at writeOrBuffer (node_modules/readable-stream/lib/internal/streams/writable.js:334:12)
      at _write (node_modules/readable-stream/lib/internal/streams/writable.js:283:10)
      at Writable.end (node_modules/readable-stream/lib/internal/streams/writable.js:539:17)
      at /Users/finwo/src/finwo/levelgraph/lib/levelgraph.js:150:18
      at /Users/finwo/src/finwo/levelgraph/lib/queryplanner.js:93:7
      at NoResultsHolder.release (node_modules/fastparallel/parallel.js:139:9)
      at process.processTicksAndRejections (node:internal/process/task_queues:77:11)

With method being values, this leads me to believe the older levelup interface is not supported by the level-read-stream package.

I'll certainly have to fix this some time, but for now I'd say that's out of scope for this PR, as the workaround supports both the older and newer interface.

@vweevers
Copy link

vweevers commented Apr 8, 2024

Was that when using level-mem? That needs to be replaced as well. I wouldn't recommend running old (e.g. level-mem) and new (e.g. level@8) side by side. See the table at https://github.com/Level/community?tab=readme-ov-file#how-do-i-upgrade-to-abstract-level

@finwo
Copy link
Member Author

finwo commented Apr 8, 2024

Correct, that was using level-mem. Using level@8 that could simply be replaced by new Level()?

@vweevers
Copy link

vweevers commented Apr 8, 2024

I assume that the tests here use level-mem as a lightweight in-memory db, while real users would more likely use level. Meaning you want those databases to have the same interface.

If so then replace:

const mem = require('level-mem')
const db = mem()

With:

const { MemoryLevel } = require('memory-level')
const db = new MemoryLevel()

@finwo
Copy link
Member Author

finwo commented Apr 8, 2024

Gotcha, will test locally and commit if that'll work including level-read-stream

@finwo finwo mentioned this pull request Apr 8, 2024
@finwo
Copy link
Member Author

finwo commented Apr 8, 2024

@mcollina Still in doubt on the changelog that scenaristaur added, as it's current state doesn't really make sense to me.

Would the current state of this PR be fine by you code-wise?

If so, I'll start on updating the readme to match the current state, and the version bump would probably have to be a major as it's breaking changes for people on older versions of leveldb.

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing Work!

CHANGELOG.md Outdated Show resolved Hide resolved
@mcollina
Copy link
Collaborator

mcollina commented Apr 9, 2024

Can you update CI to support only Node v18 an v20? I'll bump a major anyway.

@finwo
Copy link
Member Author

finwo commented Apr 9, 2024

Sure, I'll make it run 18/20

@finwo
Copy link
Member Author

finwo commented Apr 9, 2024

One more issue I have with the contributor table: it seems jez0990 has removed their github profile. Should we hunt for their current online presence or just leave it as-is?

@finwo finwo requested a review from mcollina April 12, 2024 18:26
Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit e0163eb into levelgraph:master Apr 24, 2024
3 checks passed
@finwo finwo deleted the update-level8 branch April 24, 2024 18:23
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

Successfully merging this pull request may close these issues.

None yet

4 participants