node
node copied to clipboard
nodejs
http: add writeEarlyHints function to ServerResponse
Similar to the writeContinue and writeProcessing functions, we could introduce a writeEarlyHints function where we could write the early hints response in a slightly friendlier way
response._writeRaw('HTTP/1.1 103 Early Hints\r\nLink: <\/styles.css>; rel=preload; as=style\r\n\r\n', 'ascii')
would become
response.writeEarlyHints('</styles.css>; rel=preload; as=style')
// or in case of multiple links
response.writeEarlyHints(
[
'</globals.css>; rel=preload; as=style',
'</styles.css>; rel=preload; as=style'
]
)
Problem: multiple headers
_writeRaw() cannot be used with multiple headers so I folded the headers into one line.
Link: </styles.css>; rel=preload; as=style, </scripts.js>; rel=preload; as=script
an alternative way to write this is the following
this._writeRaw('HTTP/1.1 103 Early Hints\r\n', 'ascii');
this._writeRaw('Link: </styles.css>; rel=preload; as=style\r\n', 'ascii');
this._writeRaw('Link: </scripts.js>; rel=preload; as=script\r\n\r\n', 'ascii');
however, this also folds into a oneliner, so I think prefolding it before calling _writeRaw() just once was better.
The problem is these folded headers may work now but could become deprecated in future http releases. So maybe I should just write _writeRaw() multiple times and look into unfolding or prevent the folding of the sent header elsewhere 🤔
thoughts?
Resources
- discussion about making folding obsolete in future http releases 👉 https://lists.w3.org/Archives/Public/ietf-http-wg/2015JulSep/0256.html
-
obs-fold👉 https://www.ietf.org/rfc/rfc7230.html
lgtm, solid work. It seems Chrome supports them now:
https://developer.chrome.com/blog/early-hints/
Could you add them to HTTP/2 too? Or it would be better to do it in a follow-up PR?
@mcollina good idea! I'll have to look into the http2 implementation first, I haven't read up on how the stream internals work exactly, first thought is to add it here but I'm not sure yet how it would work in a stream. Will have another look
Also what should happen when neither a string nor array is passed in? Is a 103 without headers permitted? Should we throw an exception? Either way we should also add tests for other value types.
Also what should happen when neither a string nor array is passed in? Is a 103 without headers permitted? Should we throw an exception? Either way we should also add tests for other value types.
@mscdex I added an else clause that will throw an ERR_INVALID_ARG_VALUE exception if it's not a string or an array. test case 👉 test/parallel/test-http-early-hints-invalid-argument.js
wdyt?
What should happen if the array is empty? Could you add a test for this case?
@mcollina I think there will be scenarios where we want to create a links variable, do some manipulations on that based on the resource content and then pass it on to writeEarlyHints(), so if we would have content that results in an empty array then I think we can be graceful with it and just do nothing.
As opposed to invalid types like objects (what @mscdex mentioned ☝️ ), I think that's more of a programmatic error so in that case I'm throwing ERR_INVALID_ARG_VALUE
wdyt?
Added test cases
- test/parallel/test-http-early-hints-empty-array.js
- test/parallel/test-http-early-hints-invalid-argument.js
minor nit: perhaps we could consolidate the tests into one file for this new feature?
Additionally we should test that the function throws when it should.
minor nit: perhaps we could consolidate the tests into one file for this new feature?
Additionally we should test that the function throws when it should.
@mscdex because the invalid argument test throws an exception where I process.exit(0) afterwards, the other test might fail. I can work around it by adding a timeout but I think it's more scalable if we separate the throwable tests to their dedicated files 🤔 I kept the 3 happy flow test in 1 file though
The first commit in PR should start with affected sybsystem, which is
http(srcmostly refers to generic changes in C++ parts of code). This can be fixed by rebasing and force-pushing, or on landing stage.
@LiviaMedeiros done, this is my first contrib to node so forgive me 😄
LGTM, thanks a lot for your contribution!
For reference, the contents of this
Linkheader are described in RFC 8288, B.2. Parsing a Link Field Value in particular. AFAICT there is no separate validation algorithm; current validation is good enough but can be improved in future PRs.
@LiviaMedeiros thank you for mentioning that document, I realise there are more attributes possible and updated the regex accordingly 👉 https://github.com/nodejs/node/pull/44180/commits/35ea4b90bb32364d1287c45d53ccebd60b217961
only for the UTF8 type attribute I'm not sure we can support that?
also updated my regex test suite 👉 https://regexr.com/6rjub might be better to have some tests for all current validators
lgtm, solid work. It seems Chrome supports them now: https://developer.chrome.com/blog/early-hints/ Could you add them to HTTP/2 too? Or it would be better to do it in a follow-up PR?
@mcollina good idea! I'll have to look into the http2 implementation first, I haven't read up on how the stream internals work exactly, first thought is to add it here but I'm not sure yet how it would work in a stream. Will have another look
@mcollina as promised, I read up on the implementation of the http/2 protocol in node.js, I added the writeEarlyHints() function to the compatibility api where I just use the stream.additionalHeaders() function to add it to the stream 👉 lib/internal/http2/compat.js
lgtm
Amazing work!
I think you should add to the docs in https://nodejs.org/api/http2.html#class-http2http2serverresponse too
done! 👉 https://github.com/nodejs/node/pull/44180/files#diff-f63531002261f56a8c6723723d2b7e6d05ea9ec8d433195fdf7c47728f6e3700
Why is the API restricted to only the Link header? The spec allows any header and I was about to use this for an intenral API to include some metadata and saw that it's only usable for Link. Would a PR be accepted to allow the argument to also be an object that is just a kvp of headers?
Sure. For example I'm thinking
response.writeEarlyHints({
link: 'your link header',
x-trace-id: 'id for diagnostics'
})
That would end up writing out
HTTP/1.1 103 Early Hints
link: your link header
x-trace-id: id for diagnostics
Spec: https://datatracker.ietf.org/doc/html/rfc8297
Given this has not shipped in a release yet, we can still change it. I added a dont-land-on-v18.x for now.
The API you are mentioning makes total sense.
Do browsers (or API gateways) make any use of those other headers?
AFAIK web browsers will only use the Link header (but I cannot say for sure as I am not an expert there), but what headers are in use by a client depend on the client. In our case, this is an internal API and we made our client use the trace header. This was helpful so we still have the trace header in the case the tcp connection dropped or the remote server had some other issue before sending the full response.
Sweet. I got it mostly do so far. I know it is new, so not sure on the process, but should this just be in addition to the current implementation or replace it? I.e. should I make it just take an object as the argument or should I keep this existing behavior intact and have it vary on the arg type (string = link, array = links, non array object = headers)?
Cool, sounds good. That is what I was expecting, just it's a bit more work, so was trying to see if I could take a shortcut to open PR sooner, haha ;)
If the spec allows any header I would make it simple and only use a kvp object. I think validateHeaderName() and validateHeaderValue() from _http_outgoing.js can be used to validate the key-value pairs.
If the spec allows any header I would make it simple and only use a kvp object. I think
validateHeaderName()andvalidateHeaderValue()from_http_outgoing.jscan be used to validate the key-value pairs.
I agree ☝️, hard validation makes it more robust but if we have a more lenient approach everywhere else it makes sense to just have one object @dougwilson is proposing, good catch!
Cool, sounds good. That is what I was expecting, just it's a bit more work, so was trying to see if I could take a shortcut to open PR sooner, haha ;)
Then take a shortcut and let's accept only an object!
Maybe a general api like response.writeInfomation(statusCode, [, statusMessage][, headers]) for send 1xx response. If have this api, no need more api for new 1xx status code.
