-
Notifications
You must be signed in to change notification settings - Fork 16
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
Support number[] in uint8ArrayToHexString #485
Conversation
size-limit report 📦
|
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.
Thanks!
bytes.reduce((str, byte) => str + byte.toString(16).padStart(2, "0"), ""); | ||
export const uint8ArrayToHexString = (bytes: Uint8Array | number[]) => { | ||
if (!(bytes instanceof Uint8Array)) { | ||
bytes = Uint8Array.from(bytes); |
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.
nice to have: it would be nice to not mutate the provided variables but to realocate for a more functional approach (?).
cont b = bytes instanceof Unit8Array ? bytes : Uint8Array.from(bytes);
return b.reduce(...
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 doesn't really make a difference to me but since it's already merged, I'll accept your PR :-).
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.
😉
Motivation
Candid changed what used to be
Uint8Array
toUint8Array | number[]
.This means we often have to convert a field to
Uint8Array
just to get rid of the| number[]
bit.To make this slightly less painful we can make some functions accept both.
Changes
Make
uint8ArrayToHexString
accept bothUint8Array | number[]
.I considered changing the name but in that case I don't think it's worth it anymore. What do you think?
Tests
Unit test added
Todos