-
Notifications
You must be signed in to change notification settings - Fork 525
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
Dispatch compose #2795
Dispatch compose #2795
Conversation
@mcollina @metcoder95 PTAL |
40d7cdb
to
d1fb5b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty much like the concept, it will be interesting as it allows chaining and executing the dispatching one after the other, sequentially.
Just left few comments, I'll give a second look and possibly start doing a review more of the actually implementation within Redirect and Retry handler.
Do you have an idea already for the TS types?
8e2a6f4
to
9a1a398
Compare
0f22b71
to
5c1f6c2
Compare
5c1f6c2
to
789e792
Compare
Although I agree it overlaps, I'm not 100% we sure change it, they share similar concepts but have different purposes. The mock simplifies the testing quite a lot, especially for simple scenarios; using the |
As I see it we have 3 options.
|
Some ideas: dispatcher = dispatcher.intercept(redirect(redirectOpts), retry(retryOpts))
dispatcher = dispatcher.compose(redirect(redirectOpts), retry(retryOpts))
dispatcher = compose(redirect(redirectOpts), retry(retryOpts))(dispatcher)
dispatcher = intercept(redirect(redirectOpts), retry(retryOpts))(dispatcher)
dispatcher.dispatch(dispatchOpts) |
I would rename |
+1 on |
@mcollina @metcoder95 wdyt? Are we happy with this API? I'm going to leave documenting it as a TODO for merging next into main. I suspect we might iterate on this even further. |
d9ca036
to
245118d
Compare
I really like how e.g. const proxyAgent = new Agent().compose(proxyOpts) |
Can we have it on main instead? It would ease very well the transition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
Happy to port this to main
Can you open a separate PR for that and maybe add some documentation as well as update the existing interceptor documentation as deprecated? |
Having I would need to merge main into next one day, and this will make things very hard. |
Sorry that's confusing? Wouldn't cherry-picking this into main be the same thing? |
This is based on some other changes in the |
Reverted. @metcoder95 have fun with that 😄 |
* feat: new interceptors API * WIP: compose
This reverts commit 649185a.
This relates to...
Rationale
Changes
Features
Bug Fixes
Breaking Changes and Deprecations
Status