Add typescript definition files by rauno56 · Pull Request #100 · pillarjs/router

rauno56 · 2021-04-27T16:48:54Z

Rebased Added typescript definition files #76,
Brought typescript up to date,
Added all HTTP methods, as requested in the original PR,
Removed usages of any where possible,
Fixed some bugs in the definitions.

dougwilson · 2021-05-15T16:14:51Z

index.d.ts

+
+    route(prefix: PathParams): IRoute;
+    // Stack of configured routes
+    stack: Layer[];


As a general question, are the typings supposed to include all properties and methods including private ones, so should this be removed since it is not a public property?

You are right. API documented private in the code should be defined private in TS as well. I didn't pay much attention to that since

"it's JS - accessible anyways!" and

I've actually needed to access those myself.

I'll go over it and add access modifiers where appropriate next week.

@dougwilson, I looked into it a bit, but don't know a good way to implement it. I'd leave it as it is for now.

I assume the way to implement is to not have it in here at all. My understanding (which is weak, please correct me if wrong) is that the TSD is just the public API, which is why there is no mechanism to mark something as private.

Commented out handle() and stack - left Layer type because it's useful for documenting the internal API, which will be inaccessible nevertheless.

dougwilson · 2021-05-15T16:16:27Z

index.d.ts

+    mkcol: IRouterMatcher<this>;
+    move: IRouterMatcher<this>;
+    notify: IRouterMatcher<this>;
+    pri: IRouterMatcher<this>;


Is this actually a method on there?

$ node -pe 'require(".")().pri' undefined

I took that from require('http').METHODS. v14.16.1 has it at least.

Now... this is not ideal obviously and I'm sure there's a way to handle supporting different versions better, but my current approach has been "close is better than nothing".

Ah, gotcha. I was using Node.js 16.1. So that does bring up a good point: I guess this should be all the methods possible in the various Node.js versions? I wonder what other methods got removed; probably should go through all the versions this module supports and union them together.

I asked around in TS discord and they didn't recall a better way to handle it than what you proposed - adding collecting all of them from all versions.

index.d.ts

dougwilson · 2021-05-17T16:45:15Z

index.d.ts

+  type NextFunction = (err?: Error | "route" | "router") => void;
+  type Callback = (err?: Error) => void;
+
+  type ErrorRequestHandler = (err: Error, req: IncomingRequest, res: http.ServerResponse, next: NextFunction) => void;


We may not want the err to be typed as just a Error object, as in the 1.x of this router, there is no enforcement on that; using a middleware you didn't write could have a string or plain object here, which I've seen a lot. We wouldn't want someone's TS code to think they don't need some kind of guard on the err value.

What's the more appropriate type for that then? any?

That is probably fine. It does have to be truthy, though, so not sure if that helps at all.

Changed it to any.

rauno56 · 2021-05-21T17:30:26Z

Rebased and reimplemented the CI changes on GHA.

terry-au · 2021-06-21T03:24:54Z

Is there anything blocking this PR from being merged into master?

rauno56 · 2021-06-29T13:19:57Z

Except for #100 (comment), I would not block it.

dougwilson · 2021-06-29T16:10:49Z

index.d.ts

+    route?: IRoute
+  }
+
+  type RequestParamHandler = (req: IncomingRequest, res: http.ServerResponse, next: NextFunction, value: string, name: string) => void;


I tried to figure out where it went wrong in the TS, and I think this line is the culprit: inside of the router.param callback, there is req.params, req.route, and all the other things available. I think that this should be the "routed request" type here.

Changed that to RoutedRequest.

I'm leaving `any | "route" | "router"` for NextFunctions param to document the special cases, even though the union doesn't actually widen `any` further.

I'm going to leave Layer type for documentation again and because it might be useful in cases where someone bypasses to the private API(my case). For example when writing OTel instrumentations.

rauno56 force-pushed the added-types branch 2 times, most recently from f5b4ba2 to f29f620 Compare April 29, 2021 08:19

rodion-arr added the pr label Apr 29, 2021

dougwilson added the enhancement label May 15, 2021

dougwilson reviewed May 15, 2021

View reviewed changes

index.d.ts Outdated Show resolved Hide resolved

dougwilson reviewed May 15, 2021

View reviewed changes

index.d.ts Outdated Show resolved Hide resolved

dougwilson reviewed May 17, 2021

View reviewed changes

dougwilson force-pushed the master branch 2 times, most recently from 7e90c9e to 8643ec6 Compare May 19, 2021 16:51

ilikejames and others added 11 commits May 21, 2021 20:26

Add typescript definition files

654e8c7

fix: fix the IncommingRequest typo

ba7e63b

fix: type req to IncomingMessage and res to ServerResponse

0bead50

feat: sort http methods by name

1d5bb74

feat: add less common HTTP methods

8b503dc

feat: remove usages of any

4fc5c25

fix: fix places that mistakenly reuse handler signatures

2cc21cd

fix: remove the workaround for callable types

e941dc2

style: avoid using var

8b08465

fix: remove url and method since they are present in IncomingMessage

e7d495b

fix: remove invalid(/deprecated) param signature

f384d5a

rauno56 force-pushed the added-types branch from c58cf13 to f384d5a Compare May 21, 2021 17:26

dougwilson reviewed Jun 29, 2021

View reviewed changes

rauno56 added 3 commits July 15, 2021 15:48

fix: broaden error type since there are actually no guards in place

4aae496

I'm leaving `any | "route" | "router"` for NextFunctions param to document the special cases, even though the union doesn't actually widen `any` further.

fix: remove private API from the types

1cbf48b

I'm going to leave Layer type for documentation again and because it might be useful in cases where someone bypasses to the private API(my case). For example when writing OTel instrumentations.

fix: use RoutedRequest in RequestParamHandler

4a034cf

dougwilson mentioned this pull request Aug 15, 2021

Added typescript type declaration #101

Closed

This was referenced Mar 28, 2023

Add type definition #109

Closed

Add typescript definition #110

Open

wesleytodd mentioned this pull request Mar 16, 2024

EFI: Publish and maintain types expressjs/typescript-wg#1

Open

Uh oh!

Comments

Conversation

rauno56 commented Apr 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rauno56 May 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rauno56 Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rauno56 commented May 21, 2021

Uh oh!

terry-au commented Jun 21, 2021

Uh oh!

rauno56 commented Jun 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rauno56 May 16, 2021 •

edited

Loading

rauno56 Jun 29, 2021 •

edited

Loading