Add typescript definition files #100
Conversation
rauno56
force-pushed
the
added-types
branch
2 times, most recently
from
f5b4ba2
to
f29f620
Compare
index.d.ts
Outdated
|
|
||
| route(prefix: PathParams): IRoute; | ||
| // Stack of configured routes | ||
| stack: Layer[]; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Commented out handle() and stack - left Layer type because it's useful for documenting the internal API, which will be inaccessible nevertheless.
| mkcol: IRouterMatcher<this>; | ||
| move: IRouterMatcher<this>; | ||
| notify: IRouterMatcher<this>; | ||
| pri: IRouterMatcher<this>; |
There was a problem hiding this comment.
Is this actually a method on there?
$ node -pe 'require(".")().pri'
undefined
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Outdated
| type NextFunction = (err?: Error | "route" | "router") => void; | ||
| type Callback = (err?: Error) => void; | ||
|
|
||
| type ErrorRequestHandler = (err: Error, req: IncomingRequest, res: http.ServerResponse, next: NextFunction) => void; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What's the more appropriate type for that then? any?
There was a problem hiding this comment.
That is probably fine. It does have to be truthy, though, so not sure if that helps at all.
dougwilson
force-pushed
the
master
branch
2 times, most recently
from
7e90c9e
to
8643ec6
Compare
|
Rebased and reimplemented the CI changes on GHA. |
|
Is there anything blocking this PR from being merged into master? |
|
Except for #100 (comment), I would not block it. |
index.d.ts
Outdated
| route?: IRoute | ||
| } | ||
|
|
||
| type RequestParamHandler = (req: IncomingRequest, res: http.ServerResponse, next: NextFunction, value: string, name: string) => void; |
There was a problem hiding this comment.
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.
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.
anywhere possible,