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(node/buffer): utf8ToBytes should return a Uint8Array #20769

Merged
merged 17 commits into from Oct 8, 2023

Conversation

aapoalas
Copy link
Collaborator

@aapoalas aapoalas commented Oct 3, 2023

utf8ToBytes was in a different file from the other ToBytes functions, which lead to it being missed in my earlier performance refactoring of Buffer internal APIs. It was the only one (yes, I made sure :) ) like this so I decided to move it into the same helper file as the other functions.

@aapoalas aapoalas linked an issue Oct 3, 2023 that may be closed by this pull request
@aapoalas aapoalas added the node native extension related to the node-api (.node) label Oct 3, 2023
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a test that could be added or enabled in cli/tests/node_compat/config.jsonc?

@aapoalas
Copy link
Collaborator Author

aapoalas commented Oct 5, 2023

Is there a test that could be added or enabled in cli/tests/node_compat/config.jsonc?

I tried enabling all and ended up to find that there wasn't anything more to enable, at least not anything related to this issue.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, there's a few more tests enabled so that's great 👍 thanks for fixing this Aapo!

@littledivy littledivy added node compat and removed node native extension related to the node-api (.node) labels Oct 7, 2023
Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 2

@aapoalas aapoalas merged commit effb5e1 into denoland:main Oct 8, 2023
13 checks passed
@aapoalas aapoalas deleted the fix/node-buffer-fix-20762 branch October 8, 2023 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"src.subarray is not a function" in blitBuffer
3 participants