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

http.OutgoingMessage headers API #1873

Closed
ChALkeR opened this issue Jun 2, 2015 · 22 comments
Closed

http.OutgoingMessage headers API #1873

ChALkeR opened this issue Jun 2, 2015 · 22 comments
Assignees
Labels
feature request Issues that request new features to be added to Node.js. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. http Issues or PRs related to the http subsystem. stalled Issues and PRs that are stalled.

Comments

@ChALkeR
Copy link
Member

ChALkeR commented Jun 2, 2015

Atm, http.OutgoingMessage has the following headers-related methods:

  • .setHeader(name, value),
  • .getHeader(name, value),
  • .removeHeader(name).

There probably should be methods to:

All methods that modify headers should error if the headers were already sent.

Reasoning: https://gist.github.com/ChALkeR/26573ff704f987fd5304

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Jun 2, 2015
@Fishrock123
Copy link
Contributor

You may want to have a look at #772, which was originall a getAllHeaders(), but later changed focus to a getCurrentHeaders().

I didn't go through with it however, and doing it right requires re-writing far more bits of http than I was able to do.

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 2, 2015

@Fishrock123 What's the reason for making the already sent headers ungettable?

@Fishrock123
Copy link
Contributor

@ChALkeR I'm not sure, is that the current impl? I would say it maybe shouldn't? Honestly though, no clue.

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 2, 2015

Ah, I misread #772, sorry.

Edit: No, I didn't.

Reads out all headers that are already been queued but not yet sent to the client. This can only be called before headers get implicitly flushed.

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 2, 2015

Ok, so there are no objections to adding such API methods at this moment, as I understand it.
Correct?

@benjamingr
Copy link
Member

@ChALkeR in general, I think it would be interesting to explore that option. I think it'd be better if you suggested a concrete API we can discuss.

@chrisdickinson
Copy link
Contributor

Remove a single header (if headers were not already sent).

Isn't that covered by removeHeader?

Re: holding on to sent headers – I believe we avoid it on the grounds of not holding onto "informational" data that the user could grab earlier.

re: the reasoning: I'm not sure a list of _headers usage makes a good case for adding new APIs. What's the end goal of adding these APIs, given that we are unlikely to change _headers in the future? Would it be easier to document _headers?

@benjamingr
Copy link
Member

@chrisdickinson the reasoning behind putting this behind an official API is that _headers can't and doesn't warn/error if the headers were already sent which makes it a very poor API susceptible to lots of unexpected behavior.

It might be possible to change _headers to __headers and expose a _headers getter in order to keep the current API but make it safer but that might possibly break some peoples' code.

I think the point of @ChALkeR is that this is functionality people need and expect to have on OutgoingMessage and they resort to _headers because there is no standard way to do this.

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 3, 2015

@chrisdickinson Thanks. removeHeader is indeed already there, it was a bit late in my local time when I was writing this. Edited.

Documenting _headers would cause more havoc because it is not a fine API. A proper API for headers should check if the headers were already sent and error if the user tries to modify them after that. Or else it would be a debugging hell.

I understand that _headers are not going anywhere in the foreseeable future, but that doesn't mean that we should tell users to use that instead of implementing a proper api.

@brendanashworth brendanashworth added the feature request Issues that request new features to be added to Node.js. label Jun 11, 2015
@ChALkeR ChALkeR added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 19, 2015
@jonathanong
Copy link
Contributor

any node contributors have an opinion on an API? i don't mind submitting a PR if there's somewhat of a consensus

@ChALkeR ChALkeR added the discuss Issues opened for discussions and feedbacks. label Jan 19, 2016
@ChALkeR
Copy link
Member Author

ChALkeR commented Jan 19, 2016

/cc @nodejs/ctc

@Fishrock123
Copy link
Contributor

Headers API will be a bit tricky, and probably require somewhat of a re-write of headers handling.
This is mostly because:

  • You'll want to get the headers before they are sent to modify them in some way.
  • You'll need access to headers that only get set when the headers are going to be sent, which is why people currently intercept writeHead().
  • That duplicates a lot of already extremely messy and tedious headers logic.

@Fishrock123
Copy link
Contributor

So, what you're really dealing with is: Current, Sent, and Total ("all") (including on-write) headers. The API probably needs to cover that. :/

@ChALkeR ChALkeR removed the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 19, 2016
@Fishrock123
Copy link
Contributor

@ofrobots This is (one of?) the issue(s) for headers handlng refactoring.

@ChALkeR
Copy link
Member Author

ChALkeR commented Feb 26, 2017

@mscdex the only remaining usecase here is «Check if headers were already sent (like ._headerSent)». Is that doable through the public API now?

@ChALkeR ChALkeR removed the discuss Issues opened for discussions and feedbacks. label Feb 26, 2017
@dougwilson
Copy link
Member

Though there is a slight difference in the meaning, I wonder if the existing res.headersSent getter fits the use cases people are using res._headerSent for?

@ChALkeR
Copy link
Member Author

ChALkeR commented Feb 26, 2017

@dougwilson Hm. It probably doesn't make sense to include two of those. I should check then why ._headerSent is used, since .headersSent is present for quite a long time. Perhaps those usages could be replaced with .headersSent (and this issue could be closed then), or perhaps there is a difference and then we should discuss what to do next.

@Trott
Copy link
Member

Trott commented Jul 27, 2017

This should stay open, yes?

@ChALkeR
Copy link
Member Author

ChALkeR commented Jul 30, 2017

@Trott, yes, there are some things that I should check on this one before closing. Thanks for reminding me, though! :-)

@Fishrock123 Fishrock123 removed their assignment Nov 23, 2017
@Fishrock123 Fishrock123 added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Nov 23, 2017
@evanlucas
Copy link
Contributor

@ChALkeR did you ever get around to this? Is the plan to consolidate _headerSent and headersSent?

@jasnell
Copy link
Member

jasnell commented Aug 11, 2018

Should this remain open?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Aug 11, 2018
@jasnell
Copy link
Member

jasnell commented Oct 17, 2018

Closing due to lack of follow up

@jasnell jasnell closed this as completed Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. http Issues or PRs related to the http subsystem. stalled Issues and PRs that are stalled.
Projects
None yet
Development

No branches or pull requests