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

Added check to support Uint8Array in response sending #6285

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

alexandercerutti
Copy link

@alexandercerutti alexandercerutti commented Jan 18, 2025

In an attempt to convert Buffer to Uint8Array in my codebase, I found out that sending an Uint8Array to .send(), makes it send as a JSON (which is wrong).

This was also noted in a comment in #4807 and I didn't find any issue in the repo (except that answer) or in the documentation about the lack of support to Uint8Array.

So I decided to look and test for a solution.

It seems to be enough to call ArrayBuffer.isView() when the kind of chunk is checked.
Even if ArrayBuffer.isView(chunk) returns true when chunk is a Buffer, I kept the Buffer.isBuffer(chunk) check, because of support to 0.10 (ArrayBuffer.isView() seems got introduced in Node v4.0.0, according to MDN).

Let me know if you see anything I can improve.

Copy link
Member

@wesleytodd wesleytodd left a comment

Choose a reason for hiding this comment

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

From that thread, just in case anyone else comes here and is wondering what folks can do in the mean time:

Current workaround is to do Buffer.from(uint8ArrayData) before passing to send method.


because of support to 0.10

We dont support that anymore, so see the one suggestion. 🎉

lib/response.js Outdated Show resolved Hide resolved
@alexandercerutti
Copy link
Author

@wesleytodd okay great! I referred to the compatibility table in the website, but I understood that only v5 was not supporting it anymore. If v4 is not updated anymore (but security fixes and maintenance), this is fine. I've committed the suggested changes.

@wesleytodd
Copy link
Member

v4 is still supported, and we can back port this if you would like to open a PR against the 4.x branch. It was just that this PR is opened against the main line, which is now v5. If you do that, the original change should be good.

@alexandercerutti
Copy link
Author

I can that, sure! v4 is still widely used so I think it should receive such support.

Thanks for the guidance

Copy link
Member

@wesleytodd wesleytodd left a comment

Choose a reason for hiding this comment

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

Sorry I should have said this in my previous review, but I would love to see a new test for this behavior before we merge. We should ideally use the same test here and in the backport.

@alexandercerutti
Copy link
Author

I can try to do that in the next days (within the week). If I don't write here anymore, please ping me! 🙌 I have quite busy days upcoming...

@alexandercerutti
Copy link
Author

@wesleytodd I found a free moment. I created a test with Uint8Array encoding. Let me know if this works for you or if you had in mind any other ideas to test.

@mysuf
Copy link

mysuf commented Jan 20, 2025

Allow me to join your discussion. I only encountered UInt8Array issue when migrating from v4 to v5, so the previous version was sending it probably correctly. So before you jump into anything, I would double check.

@alexandercerutti
Copy link
Author

@mysuf thanks for joining. This fix actually started from the last version of v4, as I started debugging express as a dependency. Then, I compared the current code in master and I saw no differences. So I wonder how was it actually working for you...

@mysuf
Copy link

mysuf commented Jan 20, 2025

@alexandercerutti Looking at the v4 codebase and you are absolutely right. I have no clue how it could work, but I swear it did. Anyway thank you for this PR.

@bjohansebas bjohansebas requested a review from a team January 22, 2025 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants