feat: Moving types into the repo

[RobinTail]

Address https://github.com/expressjs/typescript-wg/issues/1 , labeled as "top priority".

• moving types from DT into the repository for the further maintenance
  • including the correction on removing the type for express.query — it was removed in 5.0.0-alpha.1
  • corresponding PR to DT: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/72159
• validating the types
  • using typescript-eslint (ESLint updated and configured)
  • using the CLI tool from https://arethetypeswrong.github.io/
  • using tsc and expect-type assertions based on DT tests

[socket-security[bot]]

Updated dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/[email protected] ➜ 8.57.1	Transitive: eval, shell, unsafe	+94	10.8 MB	eslintbot

View full report↗︎

[RobinTail]

I hope to attend the TSC meeting tomorrow, so we could probably discuss this approach.

[bjohansebas]

@RobinTail, the meeting is in 4 hours.

https://github.com/expressjs/discussions/issues/344

[RobinTail]

This one is merged https://github.com/DefinitelyTyped/DefinitelyTyped/pull/72159 . So that this copy is identical to @types/[email protected] now.

[RobinTail]

DT also has tests for the types and I'm thinking about having ones here as well. However, I'd need to make some adjustments in order to minimize the dependencies required for that.

[xPuls3]

Are these types identical to @types/express on NPM from DefinitelyTyped? With the latest types on @types/express@5 I have inspection errors on valid v5 code.

There are no issues at runtime or in plain JavaScript, it all runs as expected, it's only the types causing the inspection error. In plain JavaScript and type stripped TypeScript the usage is valid and throws no errors. It is only the type definitions that make TypeScript believe it is invalid.

import { Router } from "express";

// TS2350: Only a void function can be called with the new keyword.
const router = new Router();

// This does not cause any errors.
const router = Router();

For router.use it does not seem to detect the parameter types when the function returns anything but void.

// Does not error, perfectly fine.
router.use((request: Request, response: Response) => {});

// Does not error, perfectly fine.
router.use("/test", (request: Request, response: Response) => {
    return void response.json({});
});

// TS2769: No overload matches this call.
router.use("/test", (request: Request, response: Response) => {
    return response.json({});
});

// TS2769: No overload matches this call.
router.use((request: Request, response: Response) => response.json({}));

The same error also occurs with get, post, and put for the same reasons.

// Does not error, perfectly fine.
router.get("/test", (request: Request, response: Response) => {
    return void response.json({});
});

// TS2769: No overload matches this call.
router.get("/test", (request: Request, response: Response) => response.json({}));
router.post("/test", (request: Request, response: Response) => response.json({}));
router.put("/test", (request: Request, response: Response) => response.json({}));

Currently anyone running TypeScript without @types/express does not have these errors. Implementing these types as is would introduce type errors where there is currently valid code.

Or perhaps I should create an issue and point these out once types are officially in express? I'm not well versed in how these things are done. Apologies if there is a better place to voice these concerns.

[RobinTail]

Are these types identical to @types/express on NPM from DefinitelyTyped?

Yes, https://github.com/expressjs/express/pull/6382#discussion_r1994130864

// TS2769: No overload matches this call.
router.get("/test", (request: Request, response: Response) => response.json({}));

The requirement of returning void | Promise<void> (and its side effects, incl. breaking changes) was discussed here: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/70696

Implementing these types as is would introduce type errors where there is currently valid code.

It's not "valid" — it's not being validated at all, which comes from your previous statement that someone is running Typescript without having actual types installed. When @types/express is not installed, its types fall back to any which in its turn basically disables Typescript for express entities, unless noImplicitAny is set in tsconfig.json.

Valid is to return void | Promise<void> which response.json() does not. ¯\_(ツ)_/¯

Therefore, it helps either to add the void keyword ahead, or to wrap that statement in curly braces {}:

router.get("/test", (request, response) => void response.json({}));
router.get("/test", (request, response) => { response.json({}); });

In case you have an idea on improving v5 types, you can make a PR to DT, @xPuls3 , and when it's merged, I will update my PR with your improvements.

[xPuls3]

It's not "valid" — it's not being validated at all

It's valid in that its properly handled by the library in the way its used in JavaScript and during actual runtime. Types being known is not a requirement for validity in a library that does not have any types. Issues with the the unofficial types not even in the repository does not define what is valid inside the official library.

I notice you didn't mention the inspection error that occurs when using new on the router class, which again, is valid and is handled correctly, the types arbitrarily claim it is incorrect despite the imported function working as a constructor and being referred to as a class inside the documentation, this feels like an oversight.

I'd rather have the types default to "any" than have incorrect types as that defeats the purpose. I don't think incomplete types that cause errors in "valid" (see above) code should be made official unless the current issues with it are first resolved.

That's just how I see it though, others may think differently.

[RobinTail]

It won't be merged until Express 6, @xPuls3. So I think you're safe for about a decade.