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

fix: Use Uint8Array.forEach in base64FromBytes #544

Merged
merged 2 commits into from
Apr 8, 2022

Conversation

douglasc-nflx
Copy link
Contributor

Otherwise, TypeScript 4.6.2 complains:

TS2569: Type 'Uint8Array' is not an array type or a string type. Use compiler option '--downlevelIteration' to allow iterating of iterators.
184 | function base64FromBytes(arr: Uint8Array): string {
185 | const bin: string[] = [];

186 | for (const byte of arr) {
| ^^^

Otherwise, TypeScript 4.6.2 complains:

TS2569: Type 'Uint8Array' is not an array type or a string type. Use compiler option '--downlevelIteration' to allow iterating of iterators.
    184 | function base64FromBytes(arr: Uint8Array): string {
    185 |   const bin: string[] = [];
  > 186 |   for (const byte of arr) {
        |                      ^^^
@douglasc-nflx douglasc-nflx changed the title Use Uint8Array.forEach in base64FromBytes fix: Use Uint8Array.forEach in base64FromBytes Apr 4, 2022
@stephenh
Copy link
Owner

stephenh commented Apr 8, 2022

@douglasc-nflx nice, thanks for the PR! Can you run yarn bin2ts to have the integration-test files updated, which serve as our test suite?

@douglasc-nflx
Copy link
Contributor Author

Ran yarn bin2ts and updated integration test files.

@stephenh
Copy link
Owner

stephenh commented Apr 8, 2022

Looks great, thanks!

@stephenh stephenh merged commit c7641ce into stephenh:main Apr 8, 2022
stephenh pushed a commit that referenced this pull request Apr 8, 2022
## [1.110.4](v1.110.3...v1.110.4) (2022-04-08)

### Bug Fixes

* Use Uint8Array.forEach in base64FromBytes ([#544](#544)) ([c7641ce](c7641ce))
@stephenh
Copy link
Owner

stephenh commented Apr 8, 2022

🎉 This PR is included in version 1.110.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@coyotte508
Copy link
Contributor

Hello,

This PR decreases the performance of base64FromBytes on Node.JS.

Here's a benchmark: https://gist.github.com/coyotte508/a66c1c4f26e137d1f9baab78b8185bed

loop: 16.063s
forEach: 25.794s
buffer: 2.545s

The use of forEach takes 56% more time.

Using directly Buffer.from on the Uint8Array, on Node.JS, increases performance 10x from the current implem. Maybe we could avoid the whole forEach / for path when on Node.JS?

@stephenh
Copy link
Owner

stephenh commented Aug 6, 2022

@coyotte508 ah wow, yeah that's a huge difference. We have a env=node|browser|both flag, if you'd like to submit a PR to use Buffer.from that'd be great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants