GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — doc, http: add Uint8Array as allowed type by Flarna · Pull Request #45167 · nodejs/node

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji

All reactions

👍 1 reaction


          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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot added doc

Issues and PRs related to Node.js documentation.

http Issues and 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

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mcollina approved these changes

Oct 25, 2022

View reviewed changes

mcollina left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Flarna commented Oct 26, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          fixup: add version/link to PR

faf0ceb

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

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Flarna Oct 26, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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);
	}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Flarna added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label

Oct 27, 2022

aduh95 approved these changes

Oct 27, 2022

View reviewed changes

aduh95 added commit-queue

PRs queued for automated landing through the Commit Queue.

commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels

Oct 27, 2022

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. 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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Flarna deleted the http-uint8 branch

October 28, 2022 05:26

Flarna added the dont-land-on-v14.x label

Oct 28, 2022

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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

RafaelGSS mentioned this pull request

Nov 1, 2022

v19.1.0 proposal #45269

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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

danielleadams mentioned this pull request

Dec 30, 2022

v18.13.0 release proposal #46025

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 <[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


          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]>

Reviewers

jasnell

jasnell left review comments

mcollina

mcollina approved these changes

ShogunPanda

ShogunPanda approved these changes

aduh95

aduh95 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

commit-queue-squash

PRs the Commit Queue should land as one squashed commit.

doc

Issues and PRs related to Node.js documentation.

http

Issues and PRs related to the http subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

Flarna commented Oct 25, 2022

Uh oh!

nodejs-github-bot commented Oct 25, 2022

Uh oh!

ShogunPanda left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mscdex commented Oct 25, 2022

Uh oh!

Flarna commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

Flarna Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 27, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Flarna commented Oct 26, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.