-
Notifications
You must be signed in to change notification settings - Fork 642
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(encoding/unstable): add options argument to hex streaming & performance #6453
Conversation
- Bring performance to stable packages - Change how the Raw encoders and decoders work to encode inline in an existing buffer
- Merged Base32, Base32Hex, and Base32Crockford into one file. - Merged Base64, and Base64Url into one file. - Added Tests for 100% coverage. - Added JSDocs
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6453 +/- ##
=======================================
Coverage 95.32% 95.32%
=======================================
Files 577 576 -1
Lines 43417 43430 +13
Branches 6485 6487 +2
=======================================
+ Hits 41386 41400 +14
+ Misses 1992 1991 -1
Partials 39 39 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
import type { Uint8Array_ } from "./_types.ts"; | ||
export type { Uint8Array_ }; | ||
import { | ||
calcMax, |
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.
I missed this in #6480, but I don't think calcMax
should be a part of public API. What do you think?
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.
It's a utility function for the raw encode functions. Without it people would need to be familiar with the ratio between the different bases.
The name though could be better. Essentially you'd use it to calculate the minimum size your buffer needs to be when passing it to encodeRawBase64
const decoder = new TextDecoder(); | ||
return (x) => decoder.decode(x) as Expect<T>; |
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.
We can save some memory allocation by moving const decoder = new TextDecoder();
outside of decode
arrow function.
Also I think we need to pass stream: true
option to decoder.decode
call to allow partial data passed to it
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.
With moving decoder out to the top level scope, I think it would be better done in a separate pull request since this is the last one of the lot for streaming.
With the stream option, we're only sending it acsii bytes so there won't be any partial data.
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.
With moving decoder out to the top level scope, I think it would be better done in a separate pull request since this is the last one of the lot for streaming.
Fair enough
With the stream option, we're only sending it acsii bytes so there won't be any partial data.
Ok. Thanks for the clarification
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
Jumping from: #6442
Pulling the unstable hex content out into a separate pull request