Open DTrombett opened 4 days ago
This also goes in the realm of false TS assumptions, as the types are referred as optional whereas several lifecycle handles are required.
I believe we should consolidate this check in the dispatch
itself (forcing only the ones that are truly required); most of built-in (if not all) uses the base DecoratorHandler
to ensure the methods are there when used; so we will need to extend all interceptor handlers from it.
Bug Description
Most built-in handlers check if the handler passed is valid (or assume it is) instead of letting the dispatch function handle everything by default. This means, for example, that if I use an interceptor that changes the handler to add methods like
onConnect
with a built-in interceptor, I will get an error.Reproducible By
Expected Behavior
The request should be successful, and the custom interceptor handler should be used correctly
Logs & Screenshots
Environment
undici@7.0.0-alpha.6, Node v22.11.0, Windows 11 Home
Additional context
I would send a PR to address this issue, but the behavior is not the same in all interceptors so I am not sure what the expected result should be. For example, the redirect handler checks if the passed handler is completely valid and assumes that the various methods such as
onUpgrade
,onConnect
andonError
are defined: https://github.com/nodejs/undici/blob/a427e4b948c4fdae8d86a013565c3929111601b2/lib/handler/redirect-handler.js#L41 https://github.com/nodejs/undici/blob/a427e4b948c4fdae8d86a013565c3929111601b2/lib/handler/redirect-handler.js#L89 https://github.com/nodejs/undici/blob/a427e4b948c4fdae8d86a013565c3929111601b2/lib/handler/redirect-handler.js#L93 The retry handler, instead, doesn't check if the passed handler is valid but does assume that some methods are defined, likeonConnect
andonError
but notonUpgrade
: https://github.com/nodejs/undici/blob/a427e4b948c4fdae8d86a013565c3929111601b2/lib/handler/retry-handler.js#L73 https://github.com/nodejs/undici/blob/a427e4b948c4fdae8d86a013565c3929111601b2/lib/handler/retry-handler.js#L323 https://github.com/nodejs/undici/blob/a427e4b948c4fdae8d86a013565c3929111601b2/lib/handler/retry-handler.js#L90 In my opinion, no method should be assumed to be present (since it may be present in a subsequent interceptor) and thus the handler should not be checked for validity.