-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Add atob/btoa based on base64-js #776
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
Conversation
js/errors.ts
Outdated
super(msg); | ||
this.name = "InvalidCharacterError"; | ||
} | ||
} |
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.
Either add the new error to the ErrorKind
enum
Lines 29 to 73 in 52d4155
enum ErrorKind: byte { | |
NoError = 0, | |
// io errors | |
NotFound, | |
PermissionDenied, | |
ConnectionRefused, | |
ConnectionReset, | |
ConnectionAborted, | |
NotConnected, | |
AddrInUse, | |
AddrNotAvailable, | |
BrokenPipe, | |
AlreadyExists, | |
WouldBlock, | |
InvalidInput, | |
InvalidData, | |
TimedOut, | |
Interrupted, | |
WriteZero, | |
Other, | |
UnexpectedEof, | |
// url errors | |
EmptyHost, | |
IdnaError, | |
InvalidPort, | |
InvalidIpv4Address, | |
InvalidIpv6Address, | |
InvalidDomainCharacter, | |
RelativeUrlWithoutBase, | |
RelativeUrlWithCannotBeABaseBase, | |
SetHostOnCannotBeABaseUrl, | |
Overflow, | |
// hyper errors | |
HttpUser, | |
HttpClosed, | |
HttpCanceled, | |
HttpParse, | |
HttpOther, | |
} |
Or (ideally) reuse one of the existing codes. Maybe
Line 45 in 52d4155
InvalidInput, |
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.
Chose to reuse InvalidInput
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.
Looks good - one comment
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
} | ||
let result = ""; | ||
for (let i = 0; i < byteArray.length; i++) { | ||
result += String.fromCharCode(byteArray[i]); |
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.
is it better for performance to push to an array and join?
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 actually just saw an interesting benchmark on this, and by running on both Safari and Chrome, it seems for V8 string concat is faster, while JavaScriptCore runs join with better performance. There is a stackoverflow thread that discuss the internals, though it might be dated and not true any more
- Add atob() btoa() denoland#776 - Add deno.arch deno.platform denoland#773 - Add deno.symlink() and deno.symlinkSync() denoland#742 - Add deno.mkdir() and deno.mkdirSync() denoland#746 - Add deno.makeTempDir() denoland#740 - Improvements to FileInfo interface denoland#765, denoland#761 - Add fetch.blob() - Upgrade V8 to 7.0.276.15 - Upgrade Rust crates
…and#772)" (denoland#776) This reverts commit 98d52cf. CC @littledivy I had trouble updating `deno_core` in Deno as it died on `node_unit_tests::vm_test` with signal 11 during `vm context promise rejection` test. Also `Box<Rc<...>>` seems fishy as pointed as by @dsherret in denoland/deno_core#775 (comment)
Related to #775 . Uses
base64-js
for implementation (pure JS/TS)There is still another option: use a polyfill version, which might be more lightweighted.