Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ jobs:
- name: Lint code
run: npm run lint

- name: Check types
run: npm run test-types

- name: Upload code coverage
uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2
with:
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ coverage/
node_modules/
npm-debug.log
package-lock.json
*.lock
55 changes: 55 additions & 0 deletions index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import type { IncomingMessage, ServerResponse } from 'node:http'
import type {
Handler,
NextFunction,
RequestHandler,
Route as IRoute
} from './lib/route'
import Route = require('./lib/route')

export type RouterMethod<T> = <F extends Handler = RequestHandler>(path: Path, handler: F) => T

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export type RouterMethod<T> = <F extends Handler = RequestHandler>(path: Path, handler: F) => T
export type RouterMethod<T> = <F extends Handler = RequestHandler>(path: Path, handler: HandlerArg<F>, ...handlers: Array<HandlerArg<F>>) => T

For when you import handlerArgs, so these get the fix as well


export type Path = string | RegExp | Array<string | RegExp>

export interface RouterOptions {
caseSensitive?: boolean
mergeParams?: boolean
strict?: boolean
}

type MethodHandlers<T> = { [M in Method | 'all']: RouterMethod<T> }

export interface Router extends RequestHandler, MethodHandlers<Router> {
params: Record<string, any>
stack: any[]
caseSensitive: boolean
mergeParams: boolean
strict: boolean

handle(req: IncomingMessage, res: ServerResponse, callback: NextFunction): void
param(name: string, fn: (req: any, res: any, next: NextFunction, value: any, name: string) => void): this
route(path: Path): IRoute

use: {
<F extends Handler = RequestHandler>(path: Path, handler: HandlerArg<F>, ...handlers: Array<HandlerArg<F>>): Router
<F extends Handler = RequestHandler>(handler: HandlerArg<F>, ...handlers: Array<HandlerArg<F>>): Router
}

all: RouterMethod<this>
get: RouterMethod<this>
post: RouterMethod<this>
put: RouterMethod<this>
delete: RouterMethod<this>
patch: RouterMethod<this>
options: RouterMethod<this>
head: RouterMethod<this>
}

interface RouterConstructor {
(options?: RouterOptions): Router
new (options?: RouterOptions): Router
Route: typeof Route
}

declare const router: RouterConstructor
export = router

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're mixing export = with other exports, which will cause issues for anyone using skipLibCheck: false. @tsconfig/node18 enables skipLibCheck by default, which is why the CI doesn't catch this error. If you need help figuring out how to fix it, let me know.

this applies to both files.

40 changes: 40 additions & 0 deletions lib/route.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import type { IncomingMessage, ServerResponse } from 'node:http'

export type NextFunction = (err?: any) => void

export type RequestHandler = (req: IncomingMessage, res: ServerResponse, next: NextFunction) => void
export type ErrorRequestHandler = (err: Error, req: IncomingMessage, res: ServerResponse, next: NextFunction) => void

export type Handler = RequestHandler | ErrorRequestHandler

export type HandlerArg<F extends Handler = Handler> = F | ReadonlyArray<HandlerArg<F>>

export type RouteMethod<T> = <F extends Handler = RequestHandler>(handler: HandlerArg<F>, ...handlers: Array<HandlerArg<F>>) => T


export type Method =
| 'acl' | 'bind' | 'checkout' | 'connect' | 'copy' | 'delete' | 'get'
| 'head' | 'link' | 'lock' | 'm-search' | 'merge' | 'mkactivity'
| 'mkcalendar' | 'mkcol' | 'move' | 'notify' | 'options' | 'patch'
| 'post' | 'propfind' | 'proppatch' | 'purge' | 'put' | 'query'
| 'rebind' | 'report' | 'search' | 'source' | 'subscribe' | 'trace'
| 'unbind' | 'unlink' | 'unlock' | 'unsubscribe'

export type MethodHandlers<T> = { [M in Method | 'all']: RouteMethod<T> }

export interface Route extends MethodHandlers<Route> {
path: string

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like us to use the Path type from index.d.ts here as well, but unfortunately we'd end up breaking more than we'd gain, since it's been this way in the Express types for years.

stack: any[]
methods: Record<string, boolean>

_handlesMethod(method: string): boolean
_methods(): string[]
dispatch(req: IncomingMessage, res: ServerResponse, done: NextFunction): void
}

interface RouteConstructor {
new (path: Path): Route
}

declare const Route: RouteConstructor
export = Route
13 changes: 11 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
],
"license": "MIT",
"repository": "pillarjs/router",
"type": "commonjs",
"main": "index.js",
"types": "index.d.ts",
"funding": {
"type": "opencollective",
"url": "https://opencollective.com/express"
Expand All @@ -20,7 +23,11 @@
"path-to-regexp": "^8.0.0"
},
"devDependencies": {
"@arethetypeswrong/cli": "0.18.5",
"@tsconfig/node18": "18.2.6",
"@types/node": "18.19.130",
"finalhandler": "^2.1.0",
"expect-type": "1.4.0",
"mocha": "10.2.0",
"nyc": "15.1.0",
"run-series": "^1.1.9",
Expand All @@ -32,7 +39,8 @@
"LICENSE",
"HISTORY.md",
"README.md",
"index.js"
"index.js",
"index.d.ts"
],
"engines": {
"node": ">= 18"
Expand All @@ -43,6 +51,7 @@
"test:debug": "mocha --reporter spec --check-leaks test/ --inspect --inspect-brk",
"test-ci": "nyc --reporter=lcov --reporter=text npm test",
"test-cov": "nyc --reporter=text npm test",
"version": "node scripts/version-history.js && git add HISTORY.md"
"version": "node scripts/version-history.js && git add HISTORY.md",
"test-types": "tsc --noEmit && attw --pack"
}
}
118 changes: 118 additions & 0 deletions test/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
import { IncomingMessage, ServerResponse } from "node:http";
import { expectTypeOf } from "expect-type";
import Router, { ErrorRequestHandler, Method, NextFunction, Path, RequestHandler } from "..";

const router = Router();

const handler: RequestHandler = (req, res, next) => next();
const other: RequestHandler = (req, res, next) => next();

// handler parameters are inferred
router.get("/", function (req, res) {
expectTypeOf(req).toEqualTypeOf<IncomingMessage>();
expectTypeOf(res).toEqualTypeOf<ServerResponse>();
});

router.get("/", function (req, res, next) {
expectTypeOf(req).toEqualTypeOf<IncomingMessage>();
expectTypeOf(res).toEqualTypeOf<ServerResponse>();
expectTypeOf(next).toBeFunction();
});

// typescript has no way to infer the types automatically
router.get<ErrorRequestHandler>("/", function (err, req, res, next) {
Comment on lines +22 to +23

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, typescript can not automatically infer that the callback type is ErrorRequestHandler so if you don't provide the type param, it'll just infer it's params as any

The only way to solve this is to change everything completely, changing callback's param to an object like { err, req, res, next }, instead of 4 different params, this is what hono, elysiajs and other do

This would obviously require a new major so I haven't touched anything there

So just letting know that the current logic cannot be fully expressed in typescript without manually providing the type parameter on the user's side

expectTypeOf(err).toEqualTypeOf<Error>();
expectTypeOf(req).toEqualTypeOf<IncomingMessage>();
expectTypeOf(res).toEqualTypeOf<ServerResponse>();
expectTypeOf(next).toBeFunction();
});
Comment thread
gameroman marked this conversation as resolved.

// methods accept multiple handlers and nested arrays (flattened at runtime)
router.get("/", handler, other);
router.get("/", [handler, other]);
router.get("/", [handler, [other]], handler);

// at least one handler is required (runtime throws TypeError)
// @ts-expect-error
router.get("/");
// handlers must be functions or arrays of them
// @ts-expect-error
router.get("/", "not-a-handler");

// paths may be a string, RegExp, or array of either
router.get(/^\/x/, handler);
router.get(["/a", /^\/b/], handler);
// @ts-expect-error
router.get(42, handler);

// use: with or without path, multiple handlers, arrays
router.use(handler);
router.use(handler, other);
router.use([handler, [other]]);
router.use("/a", handler, [other], handler);
router.use(["/a", "/b"], handler);
router.use(/^\/c/, handler);
// @ts-expect-error path without any handler (runtime throws TypeError)
router.use("/x");
// @ts-expect-error
router.use();

// verb methods and use return the router for chaining
expectTypeOf(router.get("/", handler)).toEqualTypeOf(router);
router.use(handler).use("/a", other).get("/", handler);

// one method per node:http METHODS entry, not just the classic 8
router.trace("/", handler);
router.search("/", handler);
router.propfind("/", handler);
router["m-search"]("/", handler);
// @ts-expect-error not an HTTP method
router.foo("/", handler);

// error handlers are distinguished by the explicit generic
router.use<ErrorRequestHandler>(function (err, req, res, next) {
expectTypeOf(err).toEqualTypeOf<Error>();
next(err);
});

// param callbacks receive typed req/res
router.param("id", function (req, res, next, value, name) {
expectTypeOf(req).toEqualTypeOf<IncomingMessage>();
expectTypeOf(res).toEqualTypeOf<ServerResponse>();
expectTypeOf(next).toEqualTypeOf<NextFunction>();
expectTypeOf(name).toBeString();
});

// router is itself a request handler (mountable in a server or another router)
expectTypeOf(router).toExtend<RequestHandler>();
router.use(Router());

// the instance, options, and helper types are nameable by consumers
const mounted: Router.Router = Router({ strict: true, caseSensitive: true, mergeParams: true });
mounted.use(handler);
const opts: Router.RouterOptions = { strict: true };
Router(opts);
new Router(opts);
// @ts-expect-error unknown options are rejected
Router({ nope: true });
expectTypeOf<Method>().toExtend<string>();
expectTypeOf<Path>().toEqualTypeOf<string | RegExp | Array<string | RegExp>>();

// routes: all verbs, multiple handlers, chaining
const route = router.route("/things/:id");
expectTypeOf(route.path).toBeString();
route.all(handler).get(handler, other).post([handler, [other]]);
route.checkout(handler);
route["m-search"](handler);
// @ts-expect-error at least one handler is required
route.put();

// Route must be constructed with new, and accepts any Path
new Router.Route("/x");
new Router.Route(/^\/x/);
new Router.Route(["/a", /^\/b/]);
// @ts-expect-error calling without new does not construct (strict mode)
Router.Route("/x");
// @ts-expect-error
new Router.Route(42);

7 changes: 7 additions & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"extends": "@tsconfig/node18/tsconfig.json",
"include": ["index.d.ts", "lib/route.d.ts", "test/types.ts"],
"compilerOptions": {
"types": ["node"]
}
}