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

Missing Http2ServerResponse.setHeaders() fixed #51576

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Imperat
Copy link

@Imperat Imperat commented Jan 27, 2024

This fixes missing setHeaders:

Make it possible to invoke response.setHeaders() on
response object from `node:http2`

Fixes: nodejs#51573
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Jan 27, 2024
@Imperat Imperat changed the title Missing Http2ServerResponse.setHeaders() fixed Missing Http2ServerResponse.setHeaders() fixed Jan 27, 2024
doc/api/http2.md Outdated
Comment on lines 3943 to 3944
`headers` must be an instance of [`Headers`][] or `Map`,
if a header already exists in the to-be-sent headers,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`headers` must be an instance of [`Headers`][] or `Map`,
if a header already exists in the to-be-sent headers,
`headers` must be an instance of [`Headers`][] or `Map`.
If a header already exists in the to-be-sent headers,

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback 🙌
Fixed here

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 27, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 27, 2024
@nodejs-github-bot
Copy link
Collaborator

@mcollina
Copy link
Member

Can you fix linting and the first commit message?

@avivkeller avivkeller added the stalled Issues and PRs that are stalled. label Apr 29, 2024
Copy link
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@avivkeller
Copy link
Member

(waiting for action from author)

@jakecastelli
Copy link
Member

jakecastelli commented Aug 17, 2024

Funny enough this PR's github action got stuck for some reasons and therefore stall action to close the PR never happened. Since this is an oz fellow, I reached out to him to see if he's still interested to continue his PR, if not I will manually close it.

update: @Imperat replied to me and still interested to move the PR forward 👍

@jakecastelli jakecastelli removed the stalled Issues and PRs that are stalled. label Aug 18, 2024
@Imperat
Copy link
Author

Imperat commented Aug 18, 2024

Hi @jakecastelli @mcollina
Thanks for having a look at this PR 🙏
I've updated linting, merged the latest main branch and applied suggestions from @meyfa

Hi @mcollina

Can you fix linting and the first commit message?

Do you have a link to a doc describing how should look the commit message? I did not find anything relevant in docs.

@Imperat Imperat requested a review from meyfa August 18, 2024 06:12
@jakecastelli
Copy link
Member

jakecastelli commented Aug 18, 2024

Hi @Imperat, first commit message was referred to the project contributing commit guideline. In this PR, the subsystem should be http2 and the commit could be http2: add missing setHeaders (just a suggestion).

Also, the project requires rebase than merge, would you mind rebasing instead of merging and squash your changes into a single commit before you fix the first commit message, if you need any help please let me know.

@avivkeller
Copy link
Member

avivkeller commented Aug 18, 2024

Funny enough this PR's github action got stuck for some reasons and therefore stall action to close the PR never happened. Since this is an oz fellow

FWIW #54425

Copy link

codecov bot commented Aug 18, 2024

Codecov Report

Attention: Patch coverage is 78.94737% with 4 lines in your changes missing coverage. Please review.

Project coverage is 87.32%. Comparing base (e4f61de) to head (7886934).
Report is 4 commits behind head on main.

Files Patch % Lines
lib/internal/http2/compat.js 78.94% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #51576      +/-   ##
==========================================
- Coverage   87.33%   87.32%   -0.01%     
==========================================
  Files         648      648              
  Lines      182321   182340      +19     
  Branches    34971    34981      +10     
==========================================
+ Hits       159222   159223       +1     
- Misses      16374    16388      +14     
- Partials     6725     6729       +4     
Files Coverage Δ
lib/internal/http2/compat.js 96.50% <78.94%> (-0.34%) ⬇️

... and 25 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants