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

undici - next #2835

Closed
6 of 13 tasks
ronag opened this issue Feb 25, 2024 · 22 comments
Closed
6 of 13 tasks

undici - next #2835

ronag opened this issue Feb 25, 2024 · 22 comments
Labels
enhancement New feature or request

Comments

@ronag
Copy link
Member

ronag commented Feb 25, 2024

TODO:

Refs: #2722

@ronag ronag added the enhancement New feature or request label Feb 25, 2024
@ronag
Copy link
Member Author

ronag commented Feb 25, 2024

@metcoder95

@metcoder95
Copy link
Member

With dispatcher hooks do you refer to interceptors?

@ronag
Copy link
Member Author

ronag commented Feb 25, 2024

With dispatcher hooks do you refer to interceptors?

It's methods on Dispatcher, i.e. onHeaders etc...

@ronag
Copy link
Member Author

ronag commented Feb 25, 2024

Some of the above we can probably land on main.

@metcoder95
Copy link
Member

Should we split onHeaders into more fine-grained hooks? i.e. onHeader(key, val) and onStatus(code, message)?

This seems interesting, in theory, the HTTP response is split into three parts, the status line, headers and body; maybe we can follow this line?
See potential use cases, specially around stop request processing if the status is not the one expected

@ronag
Copy link
Member Author

ronag commented Feb 25, 2024

This seems interesting, in theory, the HTTP response is split into three parts, the status line, headers and body; maybe we can follow this line?

it's actually headers first, then status and then body.

@metcoder95
Copy link
Member

I'm working on the dump to interceptor, but I'm curious what do you have in mind for that API?

Also removing the legacy interceptor concepts from next, but is out of sync, should I open aim the branch anyways in this state?

@metcoder95
Copy link
Member

cc: @ronag

@ronag
Copy link
Member Author

ronag commented Apr 7, 2024

I will sort the next branch sometime in the next two weeks. Just open or against current state.

@ronag
Copy link
Member Author

ronag commented Apr 7, 2024

I'm working on the dump to interceptor, but I'm curious what do you have in mind for that API?

.compose(interceptors.dump({ maxSize: 1024 * 1024 }))

Something like that?

@metcoder95 metcoder95 pinned this issue Apr 14, 2024
@metcoder95 metcoder95 mentioned this issue Apr 14, 2024
7 tasks
@PandaWorker
Copy link

It is also necessary to move the decompression of compressed data (gzip, deflate, br, zstd) into a separate interceptor

@ronag
Copy link
Member Author

ronag commented Jun 21, 2024

This plans need to be redone.

@ronag ronag closed this as completed Jun 21, 2024
@ronag
Copy link
Member Author

ronag commented Jun 21, 2024

Creating a next branch etc is maybe not something I should be in charge of. I believe we should do the following things in next semvar major:

  • remove old interceptor pattern
  • remove maxRedirect defaults from agent etc... (apply interceptor in API methods instead)
  • update llhttp

And that's it. I'll probably fork and apply things there for my own needs. Happy to help if someone else takes charge.

@metcoder95
Copy link
Member

metcoder95 commented Jun 21, 2024

  • Old pattern
  • Replace max redirect with interceptor
  • Update llhttp

@metcoder95
Copy link
Member

I can help with this if support is needed 👍

@KhafraDev
Copy link
Member

KhafraDev commented Jun 24, 2024

could we add #3366 and #2727 to next too?

... or the main branch. Whatever we're doing now, I'd love to get rid of these.

@metcoder95
Copy link
Member

Yeah, let's add them to next 👍

@ronag
Copy link
Member Author

ronag commented Jun 25, 2024

@metcoder95 Just to be clear. I won't be doing anything further with the next branch despite previous indications. Feel free to do whatever you want with it. I'm doing my stuff on a fork.

@metcoder95
Copy link
Member

Sure thing, no worries 👍

@artur-ma
Copy link
Contributor

artur-ma commented Jun 26, 2024

@ronag
How about changing the onHeaders ? Now u get there just Buffer array, which requires each handler that needs headers to parse them again and again(e.g retry handler, redirect handler)

So basically, we parse the headers multiple times, in each handler that needs headers
Also regarding removing onConnect, how to measure timing of acquiring the socket without it?
and the last one, removing onBodySent and wrapping the body, is there an example for that?

Thanks

@ronag
Copy link
Member Author

ronag commented Jun 26, 2024

How about changing the onHeaders ? Now u get there just Buffer array, which requires each handler that needs headers to parse them again and again(e.g retry handler, redirect handler)

This can solves with the following pattern that I've been using:

onHeaders(statusCode, rawHeaders, resume, statusMessage, headers = parseHeaders(rawHeaders)) {

and then make sure to pass the 4th argument to any sub handlers.

Not great though. I would also prefer to have the 2nd arg already pre-parsed.

@metcoder95
Copy link
Member

Any specific reason why we added them unparsed?

@mcollina mcollina unpinned this issue Jul 22, 2024
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

5 participants