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

feat!: enable H2 by default #2796

Closed
wants to merge 1 commit into from
Closed

feat!: enable H2 by default #2796

wants to merge 1 commit into from

Conversation

metcoder95
Copy link
Member

@metcoder95 metcoder95 commented Feb 21, 2024

This relates to...

Closes #2750

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm strongly against this. I think HTTP2 for most cases is worse both in terms of performance and stability. I'm not a big fan of the quality and lack of maintenance of the http2 module in node core. I would like to even further decouple the http2 stuff from undici core. I would be happy to reconsider this position if and when James's http/3 and quic stuff lands.

@ronag
Copy link
Member

ronag commented Feb 21, 2024

I would suggest we enable it by default for fetch and only fetch. It's slow anyway :).

@metcoder95
Copy link
Member Author

I believe that most of the inquiries come from fetch, so I am happy to scope it for it only 🙂

@metcoder95
Copy link
Member Author

Hmm, taking another look that won't be that easy; as if we try to scope an Agent just for fetch, that can overlap with the getGlobalDispatcher; for instance we will need to agree if we set the global dispatcher to use H2 by default or not.

Unless there's another way I might be missing

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm -1 with this implementation.

I think a better approach would be to enable it only if the server does not advertise HTTP/1.1 in the TLS exchange. In other terms, if a server is only exposed via HTTP/2, we can support it.

@mcollina
Copy link
Member

I think we are not ready for turning this on for all requests, there are bugs: #2801.

@metcoder95
Copy link
Member Author

SGTM, let's take this back and circle once we feel we are more prepared to it; I'll revisit the bug and seek for support and possible add a fix there 👍

@metcoder95 metcoder95 closed this Feb 21, 2024
@Uzlopak Uzlopak deleted the feat/h2_default branch February 26, 2024 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants