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

feat(node/http): minor HTTP Server/Response improvements #1448

Merged
merged 2 commits into from
Oct 29, 2021
Merged

feat(node/http): minor HTTP Server/Response improvements #1448

merged 2 commits into from
Oct 29, 2021

Conversation

talentlessguy
Copy link
Contributor

This PR adds a few improvements (sorry that multiple changes in one PR, I was just fixing a lot of stuff at the same time)

ref: https://nodejs.org/api/http.html

Server

  • extends from EventEmitter and has close and connection events
  • has a close() method which triggers listener.close()

ServerResponse

  • has statusCode and statusMessage properties
  • uses web Headers API instead of a string to string record for headers management
  • added removeHeader, getHeaderNames, hasHeader
  • ensureHeaders is a private method now because it's not defined in Node.js HTTP docs
  • setHeader now returns response object for chaining (as defined in Node.js HTTP docs)

@CLAassistant
Copy link

CLAassistant commented Oct 22, 2021

CLA assistant check
All committers have signed the CLA.

@bartlomieju bartlomieju requested a review from AaronO October 22, 2021 20:38
Copy link
Contributor

@AaronO AaronO left a comment

Choose a reason for hiding this comment

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

Some overlap with #1435, would be best to merge both improvements.

@bartlomieju
Copy link
Member

@talentlessguy could you please combine changes from #1435 in your PR? I would also be great to have some tests covering the changes, especially for all those header methods.

node/http.ts Outdated Show resolved Hide resolved
@AaronO
Copy link
Contributor

AaronO commented Oct 24, 2021

@talentlessguy Do you mind if I push http changes from #1435 to this branch ? (or maybe it's best to just land this and cherrypick the #1435 changes in another PR)

Note that class field initializers absolutely murder performance, given the importance of the HTTP-stack hot paths, we're quite sensitive to this. An improvement has been landed in v8 (https://chromium.googlesource.com/v8/v8/+/713ebae3b4ef42d00220097ab2f238ffe8e4b87e), I haven't tested it yet, I don't think it closes the entire gap, ideally we should fix this upstream in v8 instead of working around it.

@talentlessguy
Copy link
Contributor Author

@AaronO yeah it's completely fine

Copy link
Contributor

@AaronO AaronO left a comment

Choose a reason for hiding this comment

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

LGTM

Note: field initializers are atrociously slow, but there are improvements coming in v8 9.7, so we'll punt on them until testing them

@AaronO AaronO merged commit 618f7ad into denoland:main Oct 29, 2021
@talentlessguy talentlessguy deleted the feat-improve-node-http branch October 29, 2021 09:51
traceypooh pushed a commit to traceypooh/deno_std that referenced this pull request Nov 14, 2021
Co-authored-by: Aaron O'Mullan <aaron.omullan@gmail.com>
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.

4 participants