nodejs / undici

An HTTP/1.1 client, written from scratch for Node.js
https://nodejs.github.io/undici
MIT License
6.09k stars 529 forks source link

`fetch` support for HTTP/2 by default #2750

Open mctrafik opened 7 months ago

mctrafik commented 7 months ago

What is the problem this feature will solve?

This will get node's implementation of fetch closer to how it behaves in the browser.

What is the feature you are proposing to solve the problem?

Support protocol upgrade similar to how fetch-h2 package does. It's weird to try to use a third-party package that uses node's internal http2 package under the hood.

What alternatives have you considered?

I am now using fetch-h2 package, but much like got-fetch or node-fetch I fear it will fall out of spec and not evolve with the ecosystem.

aduh95 commented 7 months ago

Duplicate of https://github.com/nodejs/undici/issues/399. AFAICT it has already been implemented.

mctrafik commented 7 months ago

@aduh95 Can you elaborate? Does nodeJs use undici? If yes, in what version is this supported? I'm using Node 20, which is latest LTE version and fetch there doesn't support H2.

aduh95 commented 7 months ago

Does nodeJs use undici?

Correct (it is also maintained by us) https://github.com/nodejs/node/blob/109ab0a89cdeff5717a2a16edd27bafecc104cf6/doc/contributing/maintaining/maintaining-http.md#L54-L56

You can check what version of Undici your build is using in process.versions.undici.

I'm using Node 20, which is latest LTE version and fetch there doesn't support H2.

Any chance you could send a repro? So we can reopen this issue and transfer it to the Undici repo.

targos commented 7 months ago

I don't know much about HTTP/2, but here's an example that works in a browser (do it from the website itself to avoid CORS issue), while it fails in Node.js:

fetch('https://api.sandbox.push.apple.com').then(r => r.text()).then(console.log)

It's not easy to find a public URL that only works with HTTP/2.

aduh95 commented 7 months ago

Created a minimal repro:

import { createServer, constants } from 'node:http2';
import { once } from 'node:events';

const {
  HTTP2_HEADER_STATUS,
  HTTP2_HEADER_CONTENT_TYPE,
} = constants;

const server = createServer();

server.on('stream', (stream, headers) => {
  stream.respond({
    [HTTP2_HEADER_STATUS]: 200,
    [HTTP2_HEADER_CONTENT_TYPE]: 'text/plain; charset=utf-8',
    'Access-Control-Allow-Origin': '*',
  });
  stream.write('hello world');
  stream.end();
});

server.listen(3000);
await once(server, 'listening');

const response = await fetch('http://localhost:3000/');

console.assert(response.ok, response.status);
console.log(await response.text());

server.close();

I can confirm the above code works with Deno and fails with the latest main.

mcollina commented 7 months ago

HTTP/2 support in undici is experimental and not enabled by default. Note that we do not support H2, but only HTTP/2 (over TLS), due the necessary protocol selection.

To enable:

import { createServer, constants } from 'node:http2';
import { once } from 'node:events';
import { setGlobalDispatcher, Agent } from './index.js';

const {
  HTTP2_HEADER_STATUS,
  HTTP2_HEADER_CONTENT_TYPE,
} = constants;

setGlobalDispatcher(new Agent({
  allowH2: true
}));

fetch('https://api.sandbox.push.apple.com').then(r => r.text()).then(console.log)

I think we could enable it in the next major and see how it goes. Unfortunately it might be breaking in case of bugs.

metcoder95 commented 7 months ago

I believe it should be fine as soon as we keep it as experimental though enabled by default when chosen by the server.

mcollina commented 7 months ago

I believe it should be fine as soon as we keep it as experimental though enabled by default when chosen by the server.

Good Idea! Repeating it for everybody:

We want to enable HTTP/2 if it is the only protocol that the server advertises in the TLS exchange, but prefer HTTP/1.1 for everything else. 100% agreed.PR?

mctrafik commented 7 months ago

@mcolina Just to clraify: I'm using Http2SecureServer with allowHttp1: true to return an error on unexpectedProtocol. So sounds like my server would always return the error in this proposal.

Can this behavior be controlled by a flag?!

mcollina commented 7 months ago

@mctrafik you can already turn on http2 with:

import { setGlobalDispatcher, Agent } from 'undici';

setGlobalDispatcher(new Agent({
  allowH2: true
}));
metcoder95 commented 7 months ago

I believe it should be fine as soon as we keep it as experimental though enabled by default when chosen by the server.

Good Idea! Repeating it for everybody:

We want to enable HTTP/2 if it is the only protocol that the server advertises in the TLS exchange, but prefer HTTP/1.1 for everything else. 100% agreed.PR?

SGTM 👍

3x071c commented 6 months ago

If anyone is looking for a way to enable H2 without requiring undici (which is currently not exposed as a node built-in), here's the trick:

global[Symbol.for("undici.globalDispatcher.1")] = new global[Symbol.for("undici.globalDispatcher.1")].constructor({
    allowH2: true,
});

Cheers

hjr3 commented 5 months ago

Note that we do not support H2, but only HTTP/2

What does this mean?

From RFC 9113:

The string "h2" identifies the protocol where HTTP/2 uses Transport Layer Security (TLS)

I am trying to understand the nuance of not supporting H2, but only HTTP/2.

mcollina commented 5 months ago

I am trying to understand the nuance of not supporting H2, but only HTTP/2.

I meant H2C, which is the non-tls variant.

langtutheky commented 2 months ago
global[Symbol.for("undici.globalDispatcher.1")] = new global[Symbol.for("undici.globalDispatcher.1")].constructor({
  allowH2: true,
});

@3x071c Could you please provide the TypeScript equivalent of this code, the compiler complains Element implicitly has an 'any' type because expression of type 'symbol' can't be used to index type 'typeof globalThis'. Much appreciated.

ntwcklng commented 2 months ago
global[Symbol.for("undici.globalDispatcher.1")] = new global[Symbol.for("undici.globalDispatcher.1")].constructor({
    allowH2: true,
});

@3x071c Could you please provide the TypeScript equivalent of this code, the compiler complains Element implicitly has an 'any' type because expression of type 'symbol' can't be used to index type 'typeof globalThis'. Much appreciated.

use this: ;(global as any)[Symbol.for('undici.globalDispatcher.1')] = new (global as any)[ Symbol.for('undici.globalDispatcher.1') ].constructor({ allowH2: true, })

langtutheky commented 2 months ago

@ntwcklng the snippet does silent TypeScript compiler but apparently it does not work as @3x071c mentioned to enable H2 without requiring undici because (global as any)[Symbol.for('undici.globalDispatcher.1')] returns undefined. I am running node v20.2.0