-
Notifications
You must be signed in to change notification settings - Fork 152
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
Compression Updates #502
Compression Updates #502
Conversation
Codecov ReportBase: 100.00% // Head: 100.00% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #502 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 319 319
=========================================
Hits 319 319 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@@ -19,12 +19,12 @@ class KeyvBrotli { | |||
}; | |||
} | |||
|
|||
compress(value, options) { | |||
return this.opts.compress(value, options); | |||
compress(value) { |
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.
@jaredwray we need this options
argument in compress
or decompress
methods as brotli
and gzip
also have those options filed.
https://github.com/Kikobeats/compress-brotli/blob/master/src/index.js#L29
https://github.com/nodeca/pako/blob/master/lib/inflate.js#L101
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.
@alphmth - the goal is that you would pass the options via the constructor for this compression option.
in the constructor we go with
constructor(options) {
this.opts = options;
this.compressBrotli = compressBrotli(options);
}
then we are able to use that for each of the calls.
async decompress(value) {
const {value, expires} = this.compressBrotli.deserialize(data);
return {value: await this.compressBrotli.decompress(value), expires};
}
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
The goal with this is to update the main Keyv to handle compression in a more scalable manner and not do a ton of overwrites.
compress(value, options)
anddecompress(value, options)
compress
as it is no longer needed.