feat: Add callback context and redirect history. #698
Conversation
|
Very elegant solution. Good job! |
lib/handler/redirect.js
Outdated
| this.handler.context.history = [] | ||
| } | ||
|
|
||
| this.handler.context.history.push(new URL(this.opts.path, this.opts.origin).toString()) |
There was a problem hiding this comment.
What if handler doesn't have context?
There was a problem hiding this comment.
Can we pass context similar to how we pass opaque? also make it optional.
There was a problem hiding this comment.
I'd prefer if we don't have properties on handlers in terms of user api.
There was a problem hiding this comment.
Ok, let me see what I can do. It's also a good idea because it enables custom handlers to be customizable without requiring changes in undici internals.
I'll just clarify in docs that the context object might be modified by undici. Does it sound reasonable?
There was a problem hiding this comment.
I'll just clarify in docs that the context object might be modified by undici.
Let's try it and see.
There was a problem hiding this comment.
I'll go with user provided object.
Probably also fine. We can start with trying that.
There was a problem hiding this comment.
Why? The redirect handler creates the property on itself and then just calls onContext to give the user access to it.
Because the RedirectHandler is recreated every time (decreasing maxRedirections) when following the chain. I would then store the history in opts and then call onContext. At that point is easier to use the user object.
There was a problem hiding this comment.
So, I have analyzed both solutions.
The first one, passing the context like in opaque is not working as the opts are lost in the API handler (since we destructure there). Saving the context there is not different from what I'm doing now and it complicates the external API so I gave up on that.
The onContext approach instead has the issue that if the chain is interrupted (if maxRedirections reaches 0 and 3xx is still returned) is not called at all as it is inside RedirectHandler and not inside the Agent.
I also tried to call onContext expecting the bottom handler to return an object to modify. But this implies saving the context in the bottom handler which is what I'm doing at the moment.
I also tried to mix the approaches, no luck.
I therefore think we should keep the current implementation, unless you have better idea.
There was a problem hiding this comment.
Thanks for putting the effort into considering both approaches. I'll also have a go at this.
There was a problem hiding this comment.
Ok, keep me posted then. In the meanwhile I'll just send one more commit that checks that the handler has a context before attempting to set it.
ronag
force-pushed
the
main
branch
2 times, most recently
from
b4b20c6
to
6bee407
Compare
|
Maybe we should land this with docs and typedefs after @Ethan-Arrowood has done his iteration. I kind of told him that there would be no further API changes 😄 . |
|
What about an additional optional |
|
How is it different from the |
|
@ronag Any update on this? How are we moving forward? |
I haven't had time yet to dig into this. Hopefully more time on Thursday. |
|
I looked into it last thursday but didn't have enough time, will try again this thursday |
|
Closing in favor of #769 which is a continuation. |
This PR concludes #603 by adding support for redirections history in the response.
To guarantee extensibility and encourage other handlers, I chose to introduce in the internal API the concept of
context. All three major API methods (request,pipelineandstream) now always return acontextin the response. This context is initially just an empty object, but handlers can fill them.This is what the
RedirectHandlercurrent uses.