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

feat: Switch default compression to gzip #1267

Merged
merged 2 commits into from
May 8, 2024

Conversation

cprice404
Copy link
Contributor

Now that we have the @gomomento/sdk-nodejs-compression-zstd package,
this commit updates the default compression extensions package
to use gzip as the compression algorithm rather than zstd. This
allows us to get rid of the binary dependency that makes packaging
complicated. We made the decision to do this after benchmarking
showed very little difference in compression ratio and only a little
difference in performance between the two compression libraries;
therefore, shedding the packaging complexity for a simpler user
experience seemed like the best UX choice.

cprice404 added 2 commits May 7, 2024 16:13
Now that we have the @gomomento/sdk-nodejs-compression-zstd package,
this commit updates the default compression extensions package
to use gzip as the compression algorithm rather than zstd. This
allows us to get rid of the binary dependency that makes packaging
complicated. We made the decision to do this after benchmarking
showed very little difference in compression ratio and only a little
difference in performance between the two compression libraries;
therefore, shedding the packaging complexity for a simpler user
experience seemed like the best UX choice.
@cprice404 cprice404 requested review from nand4011, malandis, bruuuuuuuce and a team May 7, 2024 23:27
Copy link
Contributor

@malandis malandis left a comment

Choose a reason for hiding this comment

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

No blockers

Comment on lines -54 to +55
40, 181, 47, 253, 0, 96, 65, 0, 0, 109, 121, 45, 118, 97, 108, 117, 101,
31, 139, 8, 0, 0, 0, 0, 0, 2, 19, 203, 173, 212, 45, 75, 204, 41, 77, 5, 0,
58, 70, 91, 55, 8, 0, 0, 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

"@gomomento/sdk": "file:../client-sdk-nodejs",
"@mongodb-js/zstd": "1.2.0"
"@gomomento/sdk": "file:../client-sdk-nodejs"
Copy link
Contributor

@malandis malandis May 7, 2024

Choose a reason for hiding this comment

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

Since zlib is part of the node runtime, does this package need to be an external dependency then? Couldn't it be included with the nodejs client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we talked about this in the team meeting the other day. for now i'd prefer that users need to stop and consider options, so i like it being separate.

@cprice404 cprice404 merged commit 0831824 into main May 8, 2024
13 checks passed
@cprice404 cprice404 deleted the move-default-compression-extensions-to-gzip branch May 8, 2024 20:14
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.

2 participants