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

Semver Major Considerations #2722

Open
ronag opened this issue Feb 9, 2024 · 23 comments
Open

Semver Major Considerations #2722

ronag opened this issue Feb 9, 2024 · 23 comments
Labels
semver-major Features or fixes that will be included in the next semver major release

Comments

@ronag
Copy link
Member

ronag commented Feb 9, 2024

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) => {}
@ronag ronag added bug Something isn't working semver-major Features or fixes that will be included in the next semver major release and removed bug Something isn't working labels Feb 9, 2024
ronag added a commit that referenced this issue Feb 9, 2024
onBodySent on onRequestSent are footguns that easily cause bugs
when implementing logic will send multiple requests, e.g. redirect
and retry.

To achieve similar functionality wrap body into a stream and listen
to 'data' and 'end' events.

Refs: #2722
ronag added a commit that referenced this issue Feb 9, 2024
onBodySent on onRequestSent are footguns that easily cause bugs
when implementing logic will send multiple requests, e.g. redirect
and retry.

To achieve similar functionality wrap body into a stream and listen
to 'data' and 'end' events.

Refs: #2722
@metcoder95 metcoder95 changed the title Semvar Major Considerations Semver Major Considerations Feb 9, 2024
@metcoder95
Copy link
Member

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
Copy link
Member Author

ronag commented Feb 9, 2024

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

ronag added a commit that referenced this issue Feb 9, 2024
onBodySent on onRequestSent are footguns that easily cause bugs
when implementing logic will send multiple requests, e.g. redirect
and retry.

To achieve similar functionality wrap body into a stream and listen
to 'data' and 'end' events.

Refs: #2722
@Uzlopak
Copy link
Contributor

Uzlopak commented Feb 9, 2024

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

@ShogunPanda
@mcollina

@metcoder95
Copy link
Member

Sure, I can have something prepared for next week 👍

@metcoder95
Copy link
Member

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 a commit that referenced this issue Feb 9, 2024
onBodySent on onRequestSent are footguns that easily cause bugs
when implementing logic will send multiple requests, e.g. redirect
and retry.

To achieve similar functionality wrap body into a stream and listen
to 'data' and 'end' events.

Refs: #2722
ronag added a commit that referenced this issue Feb 9, 2024
onBodySent on onRequestSent are footguns that easily cause bugs
when implementing logic will send multiple requests, e.g. redirect
and retry.

To achieve similar functionality wrap body into a stream and listen
to 'data' and 'end' events.

Refs: #2722
ronag added a commit that referenced this issue Feb 9, 2024
onBodySent on onRequestSent are footguns that easily cause bugs
when implementing logic will send multiple requests, e.g. redirect
and retry.

To achieve similar functionality wrap body into a stream and listen
to 'data' and 'end' events.

Refs: #2722
ronag added a commit that referenced this issue Feb 9, 2024
ronag added a commit that referenced this issue Feb 9, 2024
@ronag ronag mentioned this issue Feb 9, 2024
7 tasks
@ronag
Copy link
Member Author

ronag commented Feb 9, 2024

Added Change hooks signature to accept objects instead of params.

@ronag
Copy link
Member Author

ronag commented Feb 9, 2024

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
Copy link
Member

mcollina commented Feb 9, 2024

Can you please target a next branch?

ronag added a commit that referenced this issue Feb 9, 2024
onBodySent on onRequestSent are footguns that easily cause bugs
when implementing logic will send multiple requests, e.g. redirect
and retry.

To achieve similar functionality wrap body into a stream and listen
to 'data' and 'end' events.

Refs: #2722
@mcollina
Copy link
Member

mcollina commented Feb 9, 2024

Regarding interceptors: how would you implement them instead?

@Uzlopak
Copy link
Contributor

Uzlopak commented Feb 9, 2024

Changed target of the llhttp PR to next.

@ronag
Copy link
Member Author

ronag commented Feb 9, 2024

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
Copy link
Member

I'd like to drop FileLike and FileReader too.

@metcoder95
Copy link
Member

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
Copy link
Member Author

ronag commented Feb 11, 2024

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
Copy link
Member

Yeah, I'll tackle it within that PR 👍

@ronag
Copy link
Member Author

ronag commented Feb 11, 2024

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

@ronag
Copy link
Member Author

ronag commented Feb 11, 2024

@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
Copy link
Member

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
Copy link
Member

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

@ronag
Copy link
Member Author

ronag commented Feb 11, 2024

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

@ShogunPanda
Copy link
Contributor

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

@ShogunPanda @mcollina

Yes, please.

@PandaWorker
Copy link

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
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major Features or fixes that will be included in the next semver major release
Projects
None yet
Development

No branches or pull requests

7 participants