Semver Major Considerations

[ronag]

Based on my experience working with undici through nxt-undici and building a more advanced client on top undici I have come to a few realizations that have semver major implications:

• onBodySent and onRequestSent are huge footguns. Remove it. (just wrap the body)
• interceptors are weird and overengineered. Remove it. (just wrap dispatchers)
• onConnect is confusing and should be renamed or something to make it more intuitive. Possibly entirely removed.
• body should support a factory method (important for retries and redirects).
• onResponseStarted. Why is onHeaders insufficient?
• maxRedirections and RedirectHandler should not be part of core/dispatcher APi. Move to the api methods.
• Change hooks signature to accept objects instead of params.
• Rename disatpcher param to dispatch which is just a function (opts, handler) => {}

[metcoder95]

onBodySent and onRequestSent are huge footguns. Remove it. (just wrap the body)

SGTM, left some thoughts on the PR 👍

interceptors are weird and overengineered. Remove it. (just wrap dispatchers)

Agree, wrapping dispatches providers' full ergonomics. The interceptors feel like a half-baked feature sadly

onConnect is confusing and should be renamed or something to make it more intuitive. Possibly entirely removed.

What's the problem that surfaces from it? Renaming it seems fair, but curious about what's the exact problem there

body should support a factory method (important for retries and redirects).

Maybe we can refactor it to a base class and do something similar as we do already for the Dispatcher?

onResponseStarted. Why is onHeaders insufficient?

I kind of remember this PR; I believe it is mostly around documenting what onHeaders can be used for beyond receiving and parsing the headers.

maxRedirections and RedirectHandler should not be part of core/dispatcher APi. Move to the api methods.

Do you mean to implement them within each of the APIs or just export a new one?

Do we have a timeline for the next major version? Or maybe we can make this list the requirements for the next major?

[ronag]

@metcoder95 do you think you could help me with a PR removing the interceptor stuff?

[Uzlopak]

If we consider undici major version, than maybe target llhttp 9 upgrade to the next branch?!

@ShogunPanda @mcollina

[metcoder95]

Sure, I can have something prepared for next week 👍

[metcoder95]

If we consider undici major version, than maybe target llhttp 9 upgrade to the next branch?!

I believe that yes, but also with the considerations from @mcollina

[ronag]

Added Change hooks signature to accept objects instead of params.

[ronag]

Regarding to what to do instead of interceptors. See the following example for nxt-undici https://github.com/nxtedition/nxt-undici/blob/main/lib/index.js#L116-L132

[mcollina]

Can you please target a next branch?

[mcollina]

Regarding interceptors: how would you implement them instead?

[Uzlopak]

Changed target of the llhttp PR to next.

[ronag]

Regarding interceptors: how would you implement them instead?

const dispatch = [
  dispatch => (opts, handler) => dispatch(opts, new LogHandler(opts, { handler, dispatch })),
  dispatch => (opts, handler) => dispatch(opts, new RedirectHandler(opts, { handler, dispatch })),
  dispatch => (opts, handler) => dispatch(opts, new RetryHandler(opts, { handler, dispatch })),
].reduce((opts, handler) => rootDispatcher.dispatch(opts, handler), factory) => factory(dispatch))

request(url, { dispatcher: { dispatch } })

Or some other way, but there is IMHO no reason for it to live in undici core.

[KhafraDev]

I'd like to drop FileLike and FileReader too.

[metcoder95]

const dispatch = [
  dispatch => (opts, handler) => dispatch(opts, new LogHandler(opts, { handler, dispatch })),
  dispatch => (opts, handler) => dispatch(opts, new RedirectHandler(opts, { handler, dispatch })),
  dispatch => (opts, handler) => dispatch(opts, new RetryHandler(opts, { handler, dispatch })),
].reduce((opts, handler) => rootDispatcher.dispatch(opts, handler), factory) => factory(dispatch))

request(url, { dispatcher: { dispatch } })

Or some other way, but there is IMHO no reason for it to live in undici core.

I'd like this to be documented to showcase possible use cases as alternatives to interceptors; after writing the RetryHandler, I've found them more powerful than the interceptors attempted to.

[ronag]

const dispatch = [

  dispatch => (opts, handler) => dispatch(opts, new LogHandler(opts, { handler, dispatch })),

  dispatch => (opts, handler) => dispatch(opts, new RedirectHandler(opts, { handler, dispatch })),

  dispatch => (opts, handler) => dispatch(opts, new RetryHandler(opts, { handler, dispatch })),

].reduce((opts, handler) => rootDispatcher.dispatch(opts, handler), factory) => factory(dispatch))

request(url, { dispatcher: { dispatch } })

Or some other way, but there is IMHO no reason for it to live in undici core.

I'd like this to be documented to showcase possible use cases as alternatives to interceptors; after writing the RetryHandler, I've found them more powerful than the interceptors attempted to.

Do you think you can add that with the or to remove interceptors?

[metcoder95]

Yeah, I'll tackle it within that PR 👍

[ronag]

Once you are done with that I have some ideas I'd like to experiment to further simplify and improve the dispatch api.

[ronag]

@metcoder95 another step we should do is to change the handlers (i.e. retry, redirect etc...) to export functions instead of classes, i.e.

export const redirect = (dispatch) => ({ ...opts, redirect }, handler) => dispatch(opts, new RedirectHandler(opts, redirect, { dispatch., handler })

export const retry = (dispatch) => ({ ...opts, retry }, handler) => dispatch(opts, retry, new RetryHandler(opts, retryOpts, { dispatch., handler })

Or something like that. A little unsure how to exactly pass along options to a specific dispatcher.

[metcoder95]

Hmm... We can draft something down the line while removing the interceptors.

Like the idea, it is similar to what we have for request and other APIs. It feels quite unergonomic having to carry the dispatcher options all over the place, shall we evaluate maybe storing it within the dispatcher instance and expose it?

[mcollina]

what's the timeline for this release? Are we targeting Node.js v22, v23, or v24?

[ronag]

I don't have a clear timeline atm. I don't think these changes should be noticable inside node.

[ShogunPanda]

If we consider undici major version, than maybe target llhttp 9 upgrade to the next branch?!

@ShogunPanda @mcollina

Yes, please.

[PandaWorker]

How about adding an easy way to implement different connectors? This is necessary in order to open connections before destonation of the server through various proxies and tunnels (http/socks and more protocols)

The current implementation does not allow the use of Dispatcher.connect to open a connection to the host, it uses the connector that is under the hood

You need to think and decide how it will be more convenient to do this

[metcoder95]

Do you have an example at hand? I think can provide a better idea of the usage.

But on top of my mind, is this is something that can be implemented with interceptors already?