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

http2 #547

Closed
ronag opened this issue Feb 10, 2021 · 14 comments
Closed

http2 #547

ronag opened this issue Feb 10, 2021 · 14 comments
Labels
enhancement New feature or request

Comments

@ronag
Copy link
Member

ronag commented Feb 10, 2021

Adding http2 might be easier than I thought.

See, https://github.com/devsnek/snekfetch/blob/master/src/node/socket.js

@ronag ronag added the enhancement New feature or request label Feb 10, 2021
@mcollina
Copy link
Member

mcollina commented Mar 2, 2021

I've recently been asked how I would approach adding HTTP2 support to Undici. The starting point should be to implement an HTTP2 version of

class Client extends EventEmitter {
on top of an HTTP2 session.

@Nytelife26
Copy link
Contributor

I will make an attempt to work on this in a fork, as HTTP/2 support is one of my only qualms with undici (great job so far team). I will post an update here with my progress and I hope to submit a PR.

@mcollina
Copy link
Member

Go for it!

@Nytelife26
Copy link
Contributor

I will be waiting on the following pull requests to complete their lifecycle before I begin, to avoid risking late-stage merge conflicts:

@mcollina
Copy link
Member

I think this could be started right now without waiting for those. I would start writing an Http2Client that wraps a Http2Session object. None of those PRs touches those internals.

@Nytelife26
Copy link
Contributor

That's fine by me, then. I just questioned whether, given that the PRs at present change the structure and workings of some parts of the client, you'd want the HTTP/2 variants to inherit those improvements and thus need to wait for a merge, but if not I can start tonight when I get home.

@mcollina
Copy link
Member

There won't be much conflict to start with. There will be as you move up to implement Http2Pool and then a cross-protocol Agent.

@Nytelife26
Copy link
Contributor

It seems like the most effective solution, then, would be to start it and then follow along with the PRs when I get to those parts so I can ensure the development of the HTTP/2 variants stays up to date and won't create any delays or merge conflicts when the time comes.

@mcollina
Copy link
Member

Yes exactly!

@Nytelife26
Copy link
Contributor

Alright, I'll start with Http2Session and Http2Client tonight then, and from then forth engage in side-by-side development with relevant pull requests. Thank you!

@mcollina
Copy link
Member

mcollina commented Mar 30, 2021

Have you got any progress @Nytelife26 ? How is it going?

@ronag
Copy link
Member Author

ronag commented Mar 30, 2021

I think with #620 this should be even easier. You just need to create a new client implementing the Dispatcher interface.

@Nytelife26
Copy link
Contributor

How is it going?

Apologies, I wasn't able to start on it just yet, been very busy with college. I'll be doing that now since I have free time. And, yes, I love what you guys did with unifying under Dispatcher, that makes life much easier.

@Nytelife26 Nytelife26 mentioned this issue Apr 9, 2021
7 tasks
@ronag ronag removed the Status: in-progress This issue/pr is currently being worked on label Jul 1, 2021
@ronag
Copy link
Member Author

ronag commented Jul 1, 2021

duplicate #399

@ronag ronag closed this as completed Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants