nodejs / undici

An HTTP/1.1 client, written from scratch for Node.js
https://nodejs.github.io/undici
MIT License
6.08k stars 529 forks source link

Add generic type for opaque object for stream() and pipeline() #3378

Closed jfhr closed 2 months ago

jfhr commented 3 months ago

This would solve...

Currently, when using stream() or pipeline(), opaque is typed as unknown in the callback, e.g:

const bufs = [];

undici.stream("https://example.com", {
    method: 'GET',
    opaque: { bufs }
}, ({ statusCode, headers, opaque }) => {
    // TypeError here: Property 'bufs' does not exist on type 'unknown'.ts(2339)
    const { bufs } = opaque;
    return new Writable({
        write(chunk, encoding, callback) {
            bufs.push(chunk)
            callback()
        }
    })
});

Checking this code with TypeScript will generate an error: Property 'bufs' does not exist on type 'unknown'.ts(2339)

If opaque was defined as generic, TypeScript could infer the type from the second argument to stream() and provide the correct type inside the callback.

The implementation should look like...

Add a generic type parameter (<TOpaque = null>) to the stream() and pipeline() type declarations as well as to RequestOptions and a few other interfaces. For example:

declare function stream<TOpaque = null>(
  url: string | URL | UrlObject,
  options: { dispatcher?: Dispatcher } & Omit<Dispatcher.RequestOptions<TOpaque>, 'origin' | 'path'>,
  factory: Dispatcher.StreamFactory<TOpaque>
): Promise<Dispatcher.StreamData>;

I have also considered...

The alternative is to keep the current type declarations and require users to always cast opaque to a certain type before using it.

Additional context

@Ethan-Arrowood , who originally added the type declarations, said in his pull request:

For simplicity, I'm keeping this as unknown. Using a generic to pass through a type is no safer than type casting an unknown value.

Maybe I'm missing something here, but it seems to me that the generic approach is safer because it enforces that the opaque object that was passed in the request options has the same type as the one received in the callback. For example, the following would currently only cause a runtime error, but with the generic approach would give a type error:

const bufs  = [];

undici.stream("https://example.com", {
    method: 'GET',
    // forgot to pass opaque
}, ({ statusCode, headers, opaque }) => {
    // error: opaque is null here
    const { bufs } = opaque;
    return new Writable({
        write(chunk, encoding, callback) {
            bufs.push(chunk)
            callback()
        }
    })
});
KhafraDev commented 3 months ago

Would you like to send a PR for this? Remember to add a test.

jfhr commented 3 months ago

Ok, I will send a PR for this