Run dedicated typescript types tests using tsd

[jankapunkt]

Summary

Due to multiple conflicts with the type signatures in our Request object, I started a little spec for types testing using tsd.
Please note, that I added dev dependencies (express, undici) for now. I am currently unsure if this is good in the long-run and we may rather extract these into the specific repos for express, koa etc.

Linked issue(s)

#356 #362

Involved parts of the project

dev-only, tests, typescript

Added tests?

yes

OAuth2 standard

not involved here

Reproduction

• checkout branch
• npm install
• npm run test:types

[mrazauskas]

@jankapunkt Can I ask why did you pick tsd? Did you consider any alternatives? For instance, did you try TSTyche (https://tstyche.org)? I am its author, so that is why your PR made me curious.

Only my opinions, here is how TSTyche could be helpful for your project:

• has describe() and it() helpers (just like in your unit tests) with .skip and .omit
• the install size is only 260kB (instead of 39MB)
• uses already installed typescript package instead of shipping a patched copy
• even better, can test agains a range of TypeScript versions (tstyche --target '>=5.4') or any specific version (i.e. does not require the typescript package to be installed at all)

Might be you have your own arguments and the decision is made already. If so, sorry about the noise.

[jankapunkt]

@mrazauskas thank you for the input. I chose tsd mostly because the famous unicorn guy with the puppy dog is working on it, haha. Addtionally I like the ease of use and convention over configuration. I also looked at your repo and it looks future proof.
I will check if it will also work the same frictionless as tsd and then decide what to use in the end.

Maybe @shrihari-prakash or @dhensby have also an opinion on this one

[mrazauskas]

Hm.. extends must be a sibling of compilerOptions. It is not a compiler option. Really strange tsd does not complain about that.

[mrazauskas]

the ease of use and convention over configuration

Indeed that is good. There are also other nice aspects of tsd, like comparing types programatically. But there are the odd sides too. For instance, if you would add some utils.test-d.ts file, it would not be picked up (only index.test-d.ts is checked). And there is no simple way to see which files the CLI does actually select for the run.

In contrary, TSTyche would select all **/*.tst.* files and would list them while running the tests. Just like any JavaScript test runner does. (And more: it would show which TypeScript version and TSConfig file were used for the run.)

I do like many aspects of tsd, but also there are many oddities that made me think I could build something better. That’s why I suggested trying out TSTyche.

(Why didn’t I try to improve tsd? I did, actually. But if you look at the repo, nobody is responding to the issues or reviewing non-essential PRs anymore. Seems like the last bug fix was merged in March 2023.)

[jankapunkt]

Blocked until #383 is merged

[jankapunkt]

#383 is merged, this is free for review now

[dhensby]

Thanks for getting this going @jankapunkt — type-level regression tests for our public .d.ts are exactly what we need to stop #356/#362-style breakage recurring, and the Request/Response/Error output-type assertions (expectType<string>(req.method) etc.) are spot on. I went deep on this; a few line-level issues are inline, each with a repro — the headline ones are that the express checks are currently vacuous, and the loosened Request constructor type asserts shapes that throw at runtime.

On the open question you raised for me — tsd vs TSTyche (cc @mrazauskas): I'd lean TSTyche. The exact problems this PR hit are ones its design avoids — tsd silently treated our untyped express import as any (vacuous tests), ignored our tsconfig, and only checks index.test-d.ts. TSTyche uses our installed TypeScript (so we can pin it and test a version range, which matters for a published .d.ts consumed across many TS versions), discovers all test files, and reports what it actually ran. tsd also looks effectively unmaintained (last bugfix ~early 2023). Honest trade-offs: tsd is simpler and already wired up here, and switching is a modest rewrite. Either way the tool is secondary — the vacuous-express and runtime-contract issues need fixing regardless of which we pick.

Two more things: +1 to your own doubt about express/undici as deps in this repo — for a framework-agnostic core, framework-specific compatibility type-tests may fit better in the express/koa wrapper repos. And this'll need a rebase; it's behind master now (e.g. the chai/mocha/etc. devdep bumps here are already superseded).

[dhensby]

This widening resolves the #356 symptom but now contradicts the runtime contract. lib/request.js requires headers, method and query (throws InvalidArgumentError otherwise), yet this type makes the whole options object optional and any — so calls that typecheck here throw at runtime:

$ node -e 'const Request = require("./lib/request"); new Request({ method: "get" })'
InvalidArgumentError: Missing parameter: `headers`
# new Request() and new Request({}) throw the same way; only { headers, method, query } constructs

The original { headers, method, query, body? } type actually matched the runtime. For #356 I'd suggest keeping the required shape but making it structurally compatible (e.g. widen the header/query value types and/or accept http.IncomingMessage) rather than erasing it to Record<string, any> — otherwise consumers lose all constructor type-safety. The same applies to the Response constructor just below (line 91).

[dhensby]

These entries (undefined, {}, { method: 'get' }) are asserted as valid Request args, but they throw at runtime — the constructor requires headers/method/query:

$ node -e 'const R = require("./lib/request"); new R({ method: "get" })'
InvalidArgumentError: Missing parameter: `headers`

So this test locks in a contract the library doesn't actually honour (it passes only because the constructor type was widened to Record<string, any> — see my note in index.d.ts). Once that type is tightened, these should be the genuinely-valid shapes only, with the invalid ones asserted via expectError, e.g. expectError(new Request({ method: 'get' })).

[dhensby]

These express.* assertions are vacuous: @types/express isn't installed and express 5 ships no type declarations, so import express from 'express' resolves to any and every express.* usage typechecks regardless of what it is. Proof:

$ npx tsc --noEmit --strict index.test-d.ts
index.test-d.ts(2,21): error TS7016: Could not find a declaration file for module 'express'. ... implicitly has an 'any' type.

$ node -e 'const e = require("express"); console.log(typeof e.Request, typeof e.Response, typeof e.Error)'
undefined undefined undefined

So express.Request/express.Response/express.Error don't exist as values (express has no Error export, and Request/Response are types, not constructors) — new express.Request(...) here and new express.Error('foo') on line 134 test nothing, and would fail to compile under real @types/express. Either add @types/express and use the correct APIs, or drop the express cases and keep the genuinely-typed undici Fetch check just below.

(tsd doesn't flag this because its default config doesn't error on the implicit-any import; it does catch real assertion errors — verified by injecting one.)

[dhensby]

+1 to @mrazauskas — extends must be a top-level sibling of compilerOptions, not inside it. As written, TypeScript silently ignores it, so @tsconfig/node16 is never applied and the dev dep has no effect. It should be:

{
  "extends": "@tsconfig/node16/tsconfig.json",
  "compilerOptions": { /* ... */ }
}

(Part of why this slipped through: tsd uses its own effective config regardless of this file.) While here — strict: false weakens the type tests, and jsx: "preserve" / target: "esnext" don't really fit this project.