http2 API documentation issues

[boneskull]

Compatibility API example

Here's the example code from the API docs in the compatibility API section:

const http2 = require('http2');
const server = http2.createServer((req, res) => {
 res.setHeader('Content-Type', 'text/html');
 res.setHeader('X-Foo', 'bar');
 res.writeHead(200, { 'Content-Type': 'text/plain' });
 res.end('ok');
});

The Content-Type header is overwritten here in writeHead(), but unless the reader knows that writeHead() merges its headers with the headers set in setHeader(), the reader may think there's something special going on here.

Suggestion: remove first call to setHeader(); ensure example is straightforward

No documentation of HTTP2_HEADER_* constants

There are many references to e.g., http2.constants.HTTP2_HEADER_STATUS. These are not listed in the constants section. Is HTTP2_HEADER_CONTENT_TYPE different than 'Content-Type'? Can these be used interchangeably? If I use the compatibility API, should I use these constants?

Suggestion: document the constants and when to use them.

[Trott]

@nodejs/http2 @nodejs/documentation

[mcollina]

Regarding the "compatibility example", I think it'd just need to be updated. it's just a bad example. I also think that we should document the "merge" behavior as described. It's quite important.

The HTTP2_HEADER_* constants should never be used IMHO. I'm not really sure why they got exposed in the first place. @jasnell might now more.

[sebdeckers]

The HTTP2_HEADER_* constants should never be used IMHO.

Wait, what now...? 😰 I followed his examples and used these constants (headers, status, etc) everywhere. What are the reasons against?

[mcollina]

Those are just normal http headers and strings. I think it's fairly instructive in not using them.

[jasnell]

There's nothing saying not to use them. I included them intentionally as a convenience. There's no problem in documenting them

[boneskull]

why wouldn't you want to use them?

[mcollina]

Why don't we have the same for require('http')? They seem totally redundant and confusing for users. Those are just http headers that should be identified by strings. So, I don't use them or recommend using them.

[boneskull]

@mcollina I mean, fwiw, I would totally use any constants with http (or any other builtin) if they were available. auto-completion ftw

[mcollina]

Maybe it’s just me then :/.

[sebdeckers]

Reasons for using the constants:

• I assumed it was some sort of obscure micro-optimisation. Never checked.
• Prevent typos in header field names (e.g. Referer vs Referrer).
• Variable names are more descriptive than status codes as bare numbers.

Still I'm fine if there is a desire to deprecate these constants. Plenty of userland alternatives on NPM.

[mildsunrise]

I also want to point out that in the 'Server Side example' snippet:

// ...
server.on('stream', (stream, headers) => {
 // stream is a Duplex
 stream.respond({
 'content-type': 'text/html',
 ':status': 200
 });
 stream.end('<h1>Hello World</h1>');
});
// ...

Isn't stream.resume() needed, to start data flowing and allow the stream to be destroyed? (like in #27863)

[mcollina]

It should not be needed, have you got a repro to show that it is needed?

[mildsunrise]

Oh, you're right! For server streams, stream.resume() is called automatically on close.
So it's only needed on client streams.

The docs on stream lifecycle should be corrected to note this, then. I'll update #28044.