-
-
Notifications
You must be signed in to change notification settings - Fork 291
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
fix: simplify snappy frame encoding #5617
Conversation
Performance Report✔️ no performance regression detected Full benchmark results
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test with pre-generated compress data? Current tests check the loop data -> compress -> uncompress -> check eq. Would be good to have data -> compress --> check eq, uncompress -> check eq, where the compressed data is generated with the previous version proven to work
We already have those tests :) |
@ChainSafe/lodestar ready for rereview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
🎉 This PR is included in v1.10.0 🎉 |
Motivation
During upgrade to node 20, I noticed that our
snappy
dependency was out of date.But worse, this dependency was hidden in our snappy frame compression library, which is a poorly maintained, untyped fork at https://github.com/chainsafe/node-snappy-stream
Description
This is a minimal implementation of snappy frame compression that doesn't rely on node streams, stream-to-it, and half-a-dozen other libraries in node-snappy-stream. Instead it uses an async generator to add the framing.
The
snappy
dependency has also been updated to v7.x.x