feat: add typescript types#197
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Hey, could you add the TypeScript test infrastructure we added in on-finished? Sorry for the delay. jshttp/on-finished#69 |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
| // typescript has no way to infer the types automatically | ||
| router.get<ErrorRequestHandler>("/", function (err, req, res, next) { |
There was a problem hiding this comment.
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
bjohansebas
left a comment
bjohansebas
left a comment
There was a problem hiding this comment.
Good start! I found a few nits while doing the code review
|
|
||
| export type Handler = RequestHandler | ErrorRequestHandler | ||
|
|
||
| export type RouteMethod<T> = <F extends Handler = RequestHandler>(handler: F) => T |
There was a problem hiding this comment.
| export type RouteMethod<T> = <F extends Handler = RequestHandler>(handler: F) => T | |
| 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 | |
To handle cases like:
router.get('/', [fn1, fn2])| } from './lib/route' | ||
| import Route = require('./lib/route') | ||
|
|
||
| export type RouterMethod<T> = <F extends Handler = RequestHandler>(path: Path, handler: F) => T |
There was a problem hiding this comment.
| 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
| <F extends Handler = RequestHandler>(path: Path, handler: F): Router | ||
| <F extends Handler = RequestHandler>(handler: F): Router |
There was a problem hiding this comment.
| <F extends Handler = RequestHandler>(path: Path, handler: F): Router | |
| <F extends Handler = RequestHandler>(handler: F): Router | |
| <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 |
| } | ||
|
|
||
| declare const router: RouterConstructor | ||
| export = router |
There was a problem hiding this comment.
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.
| export interface Route { | ||
| path: string | ||
| stack: any[] | ||
| methods: Record<string, boolean> | ||
|
|
||
| _handlesMethod(method: string): boolean | ||
| _methods(): string[] | ||
| dispatch(req: IncomingMessage, res: ServerResponse, done: NextFunction): void | ||
|
|
||
| all: RouteMethod<this> | ||
| get: RouteMethod<this> | ||
| post: RouteMethod<this> | ||
| put: RouteMethod<this> | ||
| delete: RouteMethod<this> | ||
| patch: RouteMethod<this> | ||
| options: RouteMethod<this> | ||
| head: RouteMethod<this> | ||
| } |
There was a problem hiding this comment.
This makes it easier to maintain. The only thing I don't like is that we have to maintain a manual list for each method, but that's basically a limitation of Node.js's own types. And yes, I know query isn't available in every version, but we support it anyway, so that's a trade-off we have to accept.
| export interface Route { | |
| path: string | |
| stack: any[] | |
| methods: Record<string, boolean> | |
| _handlesMethod(method: string): boolean | |
| _methods(): string[] | |
| dispatch(req: IncomingMessage, res: ServerResponse, done: NextFunction): void | |
| all: RouteMethod<this> | |
| get: RouteMethod<this> | |
| post: RouteMethod<this> | |
| put: RouteMethod<this> | |
| delete: RouteMethod<this> | |
| patch: RouteMethod<this> | |
| options: RouteMethod<this> | |
| head: RouteMethod<this> | |
| } | |
| 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 | |
| stack: any[] | |
| methods: Record<string, boolean> | |
| _handlesMethod(method: string): boolean | |
| _methods(): string[] | |
| dispatch(req: IncomingMessage, res: ServerResponse, done: NextFunction): void | |
| } |
| strict?: boolean | ||
| } | ||
|
|
||
| interface Router extends RequestHandler { |
There was a problem hiding this comment.
To implement the method changes and export it as well.
| interface Router extends RequestHandler { | |
| type MethodHandlers<T> = { [M in Method | 'all']: RouterMethod<T> } | |
| export interface Router extends RequestHandler, MethodHandlers<Router> { |
|
|
||
| export type Path = string | RegExp | Array<string | RegExp> | ||
|
|
||
| interface RouterOptions { |
There was a problem hiding this comment.
It's a good idea to export it.
| interface RouterOptions { | |
| export interface RouterOptions { |
| export type RouteMethod<T> = <F extends Handler = RequestHandler>(handler: F) => T | ||
|
|
||
| export interface Route { | ||
| path: string |
There was a problem hiding this comment.
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.
| (path: string): Route | ||
| new (path: string): Route |
There was a problem hiding this comment.
It's always constructor :)
| (path: string): Route | |
| new (path: string): Route | |
| new (path: Path): Route |
There was a problem hiding this comment.
Note that this uses the Path type from index.d.ts, which is what we should be using here as well
| import { IncomingMessage, ServerResponse } from "node:http"; | ||
| import { expectTypeOf } from "expect-type"; | ||
| import Router from ".."; | ||
| import { ErrorRequestHandler } from "../lib/route"; | ||
|
|
||
| const router = Router(); | ||
|
|
||
| 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) { | ||
| expectTypeOf(err).toEqualTypeOf<Error>(); | ||
| expectTypeOf(req).toEqualTypeOf<IncomingMessage>(); | ||
| expectTypeOf(res).toEqualTypeOf<ServerResponse>(); | ||
| expectTypeOf(next).toBeFunction(); | ||
| }); |
There was a problem hiding this comment.
Okay, these are a few more valid cases that the types should support.
| import { IncomingMessage, ServerResponse } from "node:http"; | |
| import { expectTypeOf } from "expect-type"; | |
| import Router from ".."; | |
| import { ErrorRequestHandler } from "../lib/route"; | |
| const router = Router(); | |
| 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) { | |
| expectTypeOf(err).toEqualTypeOf<Error>(); | |
| expectTypeOf(req).toEqualTypeOf<IncomingMessage>(); | |
| expectTypeOf(res).toEqualTypeOf<ServerResponse>(); | |
| expectTypeOf(next).toBeFunction(); | |
| }); | |
| 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) { | |
| expectTypeOf(err).toEqualTypeOf<Error>(); | |
| expectTypeOf(req).toEqualTypeOf<IncomingMessage>(); | |
| expectTypeOf(res).toEqualTypeOf<ServerResponse>(); | |
| expectTypeOf(next).toBeFunction(); | |
| }); | |
| // 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); | |
3 participants
No description provided.