Skip to content

remove use of this with TextDecoder #348

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.

Sign up for GitHub

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

Open
tim-smart wants to merge 2 commits into
base: master
Choose a base branch
from
Open

remove use of this with TextDecoder #348

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1729ad5
remove use of `this` with TextDecoder
tim-smart Oct 18, 2023
8313824
make charset an explicit argument to decoders.other
tim-smart Oct 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ const decoders = {
if (typeof data === 'string')
data = Buffer.from(data, 'latin1');
try {
const decoder = new TextDecoder(this);
const decoder = new TextDecoder();
Copy link
Owner

@mscdex mscdex Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This isn't the correct change. Instead,

other: (data, hint) => {

should be:

other: function(data, hint) {

because of https://github.com/mscdex/busboy/pull/348/files#diff-5dfe38baf287dcf756a17c2dd63483781b53bf4b669e10efdd01e74bcd8e780aR416

Copy link
Author

@tim-smart tim-smart Oct 18, 2023

Choose a reason for hiding this comment

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

Hmm, maybe we can make that a bit clearer? Something like:

other: (charset) => (data, hint) =>

Copy link
Owner

@mscdex mscdex Oct 18, 2023

Choose a reason for hiding this comment

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

I think using bind() will lead to better efficiency as V8 should be able to optimize better rather than returning a completely "different" function every time.

Copy link
Author

@tim-smart tim-smart Oct 18, 2023

Choose a reason for hiding this comment

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

Yes, it requires a change to the call site (in getDecoder) as well (see the latest commit). If you feel it has other issues I can revert to using bind.

Copy link
Author

@tim-smart tim-smart Oct 18, 2023

Choose a reason for hiding this comment

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

Because I am curious: https://jsbench.me/1ilnv78d75/1

Looks like .bind has some overhead and slightly loses out, at least on my machine haha. V8 must do some clever things to optimize a repetitive function creation.

image

Copy link

@charmander charmander Jan 2, 2026

Choose a reason for hiding this comment

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

That benchmark website doesn’t default to strict mode, so the slowness at the time was probably from the primitive wrapping. In strict mode, it looks like the difference between the two approaches (or .bind(null, arg)) is negligible on modern V8.

return decoder.decode(data);
} catch {}
},
Expand Down