feat: add types

[Phillip9587]

This PR adds type definitions to finalhandler. It includes the following changes:

• added types
• adding type, main and types field to package.json
• adding a tsconfig.json which extends @tsconfig/node18 from https://github.com/tsconfig/bases
• adding type tests with @arethetypeswrong/cli and expect-type and run them in the CI Pipeline

Ref: expressjs/typescript-wg#1

cc @RobinTail

[socket-security]

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	@​tsconfig/​node18@​18.2.6
	@​types/​node@​18.19.130
	@​arethetypeswrong/​cli@​0.18.2
	expect-type@​1.3.0
	typescript@​5.9.3

View full report

[Phillip9587]

@jonchurch @bjohansebas @clicktodev Could you please take a look at this PR when you get a chance? It’s part of the TypeScript WG initiative, and since we’re members, your input would be valuable.

[bjohansebas]

Actually, I think this should be targeted at version 2, since that’s the one being depended on. These changes can be backported to version 3

[Phillip9587]

@bjohansebas I recommend merging this into v3. If it turns out to be necessary, we can backport it to v2 afterwards.

[clicktodev]

the advantage of releasing this as a new major release is that people will be more likely to read the patch notes since it's a major release and they will discover that they shouldn't install third party types anymore.
the only problem with releasing the types in the existing release is that it may conflict with third party types.

[bjohansebas]

okay, so let's release it as part of version 3 then

[bjohansebas]

LGTM!

[jonchurch]

Had a question about ServerResponse vs OutgoingMessage, which is mostly a nit #69 (comment)

The undefined for error seems off, Im confident it is but not 100%

[jonchurch]

I don't think undefined is possible here.

Suggested change

	listener: (err: Error | null | undefined, msg: T) => void
	listener: (err: Error | null, msg: T) => void

[bjohansebas]

It’s a behavior I’m seeing in #87 , maybe there’s a bug? I’m still working on migrating those tests to HTTP/2.

[jonchurch]

Can you point to a reproduction of the behavior?

[bjohansebas]

const http2 = require('http2')
const onFinished = require('..')

var server = http2.createServer(function (req, res) {
        onFinished(res, function (_err, msg) {
          console.log(_err)
          assert.strictEqual(msg, res)
          server.close(done)
        })
      })

      server.listen(function () {
        var port = this.address().port
        var client = http2.connect('http://127.0.0.1:' + port)
        client.on('error', noop)
        client.on('close', noop)

        var req = client.request({ ':path': '/' })
        req.on('response', function () {})
        req.end()

        setImmediate(function () {
          client.destroy()
        })
      })

[jonchurch]

Ah, I keep not checking v3 branch.

This is why typescript rules lol. It's catching a regression in types (or at least a difference, @Phillip9587 wasundefined intentional as a way to differentiate different close scenarios?)

on-finished/index.js

Line 113 in 57ba282

onFinish()

ee-first always normalized the error arg for us to Error | null:
https://github.com/jonathanong/ee-first/blob/master/index.js#L84-L86

When v3 switched to stream.finished(), that normalization was lost. I think we should fix the v3 code to explicitly pass null here (and on line 128 for socket close), keeping the type as Error | null to match v2 behavior.

At least, unless there is a compelling reason to change the behavior, keeping it the same makes sense to me.

[Phillip9587]

Is undefined a problem here? I updated the types intentionally because it is the actual behaviour after #56. I can update the onFinish function to normalize to null if we want to keep the existing behaviour:

on-finished/index.js

Lines 97 to 101 in 57ba282

	function onFinish (error) {
	if (finished) return
	finished = true
	callback(error)
	}

But even the stream.finished() callback's error is typed as optional so it includes undefined:

function finished(
    stream: NodeJS.ReadableStream | NodeJS.WritableStream | web.ReadableStream | web.WritableStream,
    callback: (err?: NodeJS.ErrnoException | null) => void,
): () => void;

Source: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/bc2d982e44ddd1ef9b27a3e4a3f24da245d23336/types/node/stream.d.ts#L1374-L1377

I would prefer not normalizing to null and calling the callback without a parameter.

[bjohansebas]

I think it’s better to leave it as is, after all, this is just a wrapper around finished(), and this is how Node.js returns it, so we should leave it that way.