Handlers incorrectly assume some methods are defined

[DTrombett]

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

import { Agent, interceptors } from "undici";

// First example
new Agent()
    .compose(
        (dispatch) => (opts, handler) =>
            dispatch(opts, {
                ...handler,
                onConnect() {},
                onHeaders: () => true,
                onComplete() {},
            }),
        interceptors.redirect({ maxRedirections: 1 }),
    )
    .dispatch(
        { method: "GET", path: "/", origin: "https://example.com" },
        {
            onError() {},
            onData: () => true,
        },
    );
// Second example
new Agent()
    .compose(
        (dispatch) => (opts, handler) =>
            dispatch(opts, {
                ...handler,
                onConnect() {},
                onHeaders: () => true,
                onComplete() {},
            }),
        interceptors.retry(),
    )
    .dispatch(
        { method: "GET", path: "/", origin: "https://example.com" },
        {
            onError() {},
            onData: () => true,
        },
    );

Expected Behavior

The request should be successful, and the custom interceptor handler should be used correctly

Logs & Screenshots

# First example
D:\-\node_modules\undici\lib\core\util.js:515
    throw new InvalidArgumentError('invalid onConnect method')
          ^
InvalidArgumentError: invalid onConnect method
    at Object.assertRequestHandler (D:\-\node_modules\undici\lib\core\util.js:515:11)
    at new RedirectHandler (D:\-\node_modules\undici\lib\handler\redirect-handler.js:41:10)
    at Proxy.Intercept (D:\-\node_modules\undici\lib\interceptor\redirect.js:15:31)
    at file:///D:/-/test.js:14:3
    at ModuleJob.run (node:internal/modules/esm/module_job:268:25)
    at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:543:26)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:116:5) {
  code: 'UND_ERR_INVALID_ARG'
}

# Second example
D:\-\node_modules\undici\lib\handler\retry-handler.js:73
    this.handler.onConnect(reason => {
                 ^
TypeError: this.handler.onConnect is not a function
    at new RetryHandler (D:\-\node_modules\undici\lib\handler\retry-handler.js:73:18)
    at Proxy.retryInterceptor (D:\-\node_modules\undici\lib\interceptor\retry.js:9:9)
    at file:///D:/-/test.js:14:3
    at ModuleJob.run (node:internal/modules/esm/module_job:268:25)
    at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:543:26)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:116:5)

Environment

[email protected], 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 and onError are defined:

undici/lib/handler/redirect-handler.js

Line 41 in a427e4b

util.assertRequestHandler(handler, opts.method, opts.upgrade)

undici/lib/handler/redirect-handler.js

Line 89 in a427e4b

this.handler.onConnect(abort, { history: this.history })

undici/lib/handler/redirect-handler.js

Line 93 in a427e4b

this.handler.onUpgrade(statusCode, headers, socket)

The retry handler, instead, doesn't check if the passed handler is valid but does assume that some methods are defined, like onConnect and onError but not onUpgrade:

undici/lib/handler/retry-handler.js

Line 73 in a427e4b

this.handler.onConnect(reason => {

undici/lib/handler/retry-handler.js

Line 323 in a427e4b

return this.handler.onError(err)

undici/lib/handler/retry-handler.js

Line 90 in a427e4b

if (this.handler.onUpgrade) {

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.

[metcoder95]

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.

[DTrombett]

This doesn't happen anymore when using new handler methods (e.g. onRequestStart)

[mcollina]

@ronag could you take a look?

[metcoder95]

This is not reproducible anymore?

[DTrombett]

Sorry for being a bit unclear, I meant that there's no issue when using the new handler methods. The error still occurs when using the old methods like in the example but I'm not sure if it is still worth fixing as they're deprecated now