kwhitley / itty-router

A little router.
MIT License
1.7k stars 77 forks source link

Middleware functions don't seem to typecheck correctly for `request` #190

Closed alexrosenfeld10 closed 3 months ago

alexrosenfeld10 commented 8 months ago

Describe the Issue

Middleware functions are not typchecking properly. See discord thread for full discussion.

Example Router Code

Reproduction available at https://codesandbox.io/s/typescript-playground-forked-hwcnq5

import { Router, IRequestStrict, withContent, json } from "itty-router";

const router = Router();

type HasFoo = {
  foo: string;
} & IRequestStrict;

type HasBar = {
  bar: string;
} & IRequestStrict;

const withFoo = async (request: HasFoo) => {
  request.foo = "foo";
};
const withBar = async (request: HasBar) => {
  request.bar = "bar";
};

/*
 this will fail to typecheck, 
Argument of type '(request: HasBar) => Promise<void>' is not assignable to parameter of type 'RouteHandler<HasFoo, []>'.
  Types of parameters 'request' and 'request' are incompatible.
    Type 'HasFoo' is not assignable to type 'HasBar'.
      Property 'bar' is missing in type '{ foo: string; } & { method: string; url: string; route: string; params: { [key: string]: string; }; query: { [key: string]: string | string[] | undefined; }; proxy?: any; } & Request' but required in type '{ bar: string; }'.ts(2345)
*/
router.all("/*", withFoo, withBar, withContent, (request) => {
  return json({ message: "ahh! types!" });
});

Request Details

Steps to Reproduce

See link above

Expected Behavior

Types check properly

Actual Behavior

They do not 😢

Environment (please complete the following information):

See reproduction above. Copied here for convenience:

{
  "name": "typescript-playground",
  "version": "1.0.0",
  "description": "",
  "main": "index.ts",
  "dependencies": {
    "itty-router": "^4.0.23"
  },
  "devDependencies": {
    "typescript": "^5.2.2"
  }
}

Additional Context

Add any other context about the problem here.

proddy commented 6 months ago

does replacing with

router.all("/*", withFoo, withBar, withContent, (request: IRequestStrict & HasFoo & HasBar) => {

help?

alexrosenfeld10 commented 6 months ago

discussion ongoing at https://discord.com/channels/832353585802903572/832354150884442122/1185270897158926396

kwhitley commented 3 months ago

So, I will attempt a back-port of as many of the v5 features (including the updated types) to v4.x... in the meantime, we have a full TS guide in the v5 docs (itty.dev/itty-router/typescript/), which should be much more helpful than what we had.

Regarding the case of this specific issue, this is really an issue oh how types are [not] inferred across an array boundary, when one item can mutate the next.

In the case of stacked middleware, I'd recommend an approach like the following:

// example types
type ARequest = { a: string } & IRequestStrict
type BRequest = { b: string } & IRequestStrict

// middleware 1
const withA: RequestHandler<ARequest> = (request) => {
  request.a = 'foo'
}

// middleware 2
const withB = (request: BRequest) => { // alternative syntax
  request.b = 'foo'
}

// later to actually use this safely, we can pass a generic through the whole chain
.get<ARequest & BRequest>('*', withUser, withPet, (request) => {
  request.a // should find this
  request.b // should find this
})

I'm going to add this specific use-case to the middleware section of the TS guide, but close this ticket in the meantime. If we run into serious issues with the v5 types (mostly identical), let's revisit or discuss on Discord!

kwhitley commented 3 months ago

Followup: added a section/example to: https://itty.dev/itty-router/typescript/request-handlers#example-multiple-middleware-handlers

alexrosenfeld10 commented 3 months ago

@kwhitley thanks. If this is fixed in 5.x I wouldn't bother back porting, but that's just me - totally your prerogative. Just saying, I intend to do the 5.x upgrade soon so personally I don't mind if it's not back ported. Not sure how many others have this issue though.