http: add writeEarlyHints function to ServerResponse

[wingleung]

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

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[mcollina]

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?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/45935/

[ShogunPanda]

LGTM!

[wingleung]

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]

What should happen if the array is empty? Could you add a test for this case?

[mscdex]

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.

[wingleung]

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?

[wingleung]

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

[mscdex]

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.

[mcollina]

lgtm

[wingleung]

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

[LiviaMedeiros]

The first commit in PR should start with affected sybsystem, which is http (src mostly refers to generic changes in C++ parts of code). This can be fixed by rebasing and force-pushing, or on landing stage.

[mcollina]

This PR must be backported / released together with #44820.

cc @nodejs/releasers