FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc, http: add Uint8Array as allowed type by Flarna · Pull Request #45167 · nodejs/node · GitHub

/ node Public

doc, http: add Uint8Array as allowed type - #45167

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
Flarna:http-uint8
Oct 27, 2022
ConversationCommits (2)ChecksFiles changed
Merged

doc, http: add Uint8Array as allowed type#45167
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
Flarna:http-uint8

Conversation

Flarna commented Oct 25, 2022

Copy link
Copy Markdown
Member

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

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 commented Oct 25, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

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 approved these changes Oct 25, 2022
View reviewed changes

ShogunPanda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM!

mcollina approved these changes Oct 25, 2022
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

mscdex commented Oct 25, 2022

Copy link
Copy Markdown
Contributor

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

Flarna commented Oct 26, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 reviewed Oct 26, 2022
View reviewed changes
Comment thread doc/api/http.md
-->

* `data` {string|Buffer}
* `data` {string|Buffer|Uint8Array}

jasnell Oct 26, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Flarna Oct 26, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 27, 2022
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 removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 27, 2022
nodejs-github-bot merged commit 760695b into nodejs:main Oct 27, 2022

nodejs-github-bot commented Oct 27, 2022

Copy link
Copy Markdown
Collaborator

Landed in 760695b

Flarna deleted the http-uint8 branch October 28, 2022 05:26
RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
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 <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RafaelGSS mentioned this pull request Nov 1, 2022
Merged
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
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 <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
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 <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
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 <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell left review comments
mcollina mcollina approved these changes
ShogunPanda ShogunPanda approved these changes
aduh95 aduh95 approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL