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.

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

Jump to bottom

doc, http: add Uint8Array as allowed type #45167

Merged
merged 2 commits into from
Oct 27, 2022
Merged

doc, http: add Uint8Array as allowed type #45167

merged 2 commits into from
Oct 27, 2022

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Oct 25, 2022

OutgoingMessage.write()/end() and their derived classes support also Uint8Array besides string and Buffer.

@Flarna
doc, http: add Uint8Array as allowed type
9929cd6 
OutgoingMessage.write()/end() and their derived classes support also
Uint8Array besides string and Buffer.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. labels Oct 25, 2022
ShogunPanda
ShogunPanda approved these changes Oct 25, 2022
View reviewed changes
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

mcollina
mcollina approved these changes Oct 25, 2022
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mscdex
Copy link
Contributor

mscdex commented Oct 25, 2022

Perhaps we should add the first node version(s) in which they were supported as well?

@Flarna
Copy link
Member Author

Flarna commented Oct 26, 2022 •
edited
Loading

Perhaps we should add the first node version(s) in which they were supported as well?

Do you know the versions? Seems this was done long time ago and not directly in HTTP instead somewhere else.
Any hints where to start looking would be nice.

I just know that the version differs between write and end - e.g. 14.20.1 write allows it but end throws.

Update:
Found the PR: #33155 and updated docs to add this info

it doesn't work on 14.x because #33155 was backported but #31818 was not therefore end has still a Buffer || string check there.

jasnell
jasnell reviewed Oct 26, 2022
View reviewed changes
doc/api/http.md
- version: v10.0.0
pr-url: https://github.com/nodejs/node/pull/18780
description: This method now returns a reference to `ClientRequest`.
-->

* `data` {string|Buffer}
* `data` {string|Buffer|Uint8Array}
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to remember, are other TypedArray types supported also? e.g. Uint16Array, Uint32Array, etc. If so, this should likely be updated to be TypedArray rather than just Uint8Array

Copy link
Member Author

Choose a reason for hiding this comment

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

According to docs e.g. fs supports this but not http, see

node/lib/_http_outgoing.js

Lines 836 to 845 in 775bf62

if (chunk === null) {
throw new ERR_STREAM_NULL_VALUES();
} else if (typeof chunk === 'string') {
len = Buffer.byteLength(chunk, encoding);
} else if (isUint8Array(chunk)) {
len = chunk.length;
} else {
throw new ERR_INVALID_ARG_TYPE(
'chunk', ['string', 'Buffer', 'Uint8Array'], chunk);
}

@Flarna Flarna added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 27, 2022
@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Oct 27, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 27, 2022
@nodejs-github-bot nodejs-github-bot merged commit 760695b into nodejs:main Oct 27, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 760695b

@Flarna Flarna deleted the http-uint8 branch October 28, 2022 05:26
RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
@Flarna @RafaelGSS
doc, http: add Uint8Array as allowed type
3f0bfca 
OutgoingMessage.write()/end() and their derived classes support also
Uint8Array besides string and Buffer.

PR-URL: #45167
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
Merged
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
@Flarna @RafaelGSS
doc, http: add Uint8Array as allowed type
7225a7d 
OutgoingMessage.write()/end() and their derived classes support also
Uint8Array besides string and Buffer.

PR-URL: #45167
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@Flarna @danielleadams
doc, http: add Uint8Array as allowed type
4d58ab9 
OutgoingMessage.write()/end() and their derived classes support also
Uint8Array besides string and Buffer.

PR-URL: #45167
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@Flarna @danielleadams
doc, http: add Uint8Array as allowed type
c34f07a 
OutgoingMessage.write()/end() and their derived classes support also
Uint8Array besides string and Buffer.

PR-URL: #45167
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@Flarna @danielleadams
doc, http: add Uint8Array as allowed type
6fb74a9 
OutgoingMessage.write()/end() and their derived classes support also
Uint8Array besides string and Buffer.

PR-URL: #45167
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell left review comments

@aduh95 aduh95 aduh95 approved these changes

@ShogunPanda ShogunPanda ShogunPanda approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Flarna @nodejs-github-bot @mscdex @mcollina @ShogunPanda @jasnell @aduh95