Semver Major Considerations about undici HOT 20 OPEN

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

from undici.

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

@ShogunPanda
@mcollina

from undici.

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

from undici.

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

from undici.

Can you please target a next branch?

from undici.

I'd like to drop FileLike and FileReader too.

from undici.

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.

from undici.

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

from undici.

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?

from undici.

Sure, I can have something prepared for next week 👍

from undici.

Added Change hooks signature to accept objects instead of params.

from undici.

Regarding interceptors: how would you implement them instead?

from undici.

Changed target of the llhttp PR to next.

from undici.

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.

from undici.

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?

from undici.

Yeah, I'll tackle it within that PR 👍

from undici.

@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.

from undici.

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?

from undici.

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

from undici.

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

from undici.