connect: add req.originalUrl; improve API; API docs from README#40409
connect: add req.originalUrl; improve API; API docs from README#40409geekley wants to merge 3 commits intoDefinitelyTyped:masterfrom geekley:connect-originalUrl
Conversation
|
👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings. Let’s review the numbers, shall we?
Looks like there were a couple significant differences—take a look at worst-case duration for getting completions at a position to make sure everything looks ok. |
typescript-bot
added
Perf: Worse
|
@geekley Thank you for submitting this PR! 🔔 @SomaticIT @EvanHahn - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
EvanHahn
left a comment
EvanHahn
left a comment
There was a problem hiding this comment.
IMO, too many things are happening in this pull request. It seems like there are a few distinct pieces:
- Add documentation
- Add
connect.Request, which extendshttp.IncomingMessagewith anoriginalUrl - Removes a lot of TSLint stuff
Could we split these into multiple changes to make it easier to review?
| * at the route `/foo`, the request for `/foo/bar` will invoke it with `req.url === '/bar'` and | ||
| * `req.originalUrl === '/foo/bar'`. | ||
| */ | ||
| url: string; |
There was a problem hiding this comment.
This is already a property of http.IncomingMessage. Have you added this for documentation purposes?
There was a problem hiding this comment.
Yes, the documentation override is important because this url is conceptually different from the superclass (since it doesn't contain the route part). Also the not-null guarantee. It helps if you have strict null checking, you can do req.url without an assertion.
typescript-bot
added
Revision needed
|
@geekley One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you! |
| /** An optional error, to make the app skip to error-handling middleware. */ err?: any | ||
| ) => void; | ||
|
|
||
| export type SimpleHandleFunction = (req: http.IncomingMessage, res: http.ServerResponse) => void; |
There was a problem hiding this comment.
"Optional" param next in callback was removed according to do's and dont's (it's always available to callbacks), so SimpleHandleFunction was deprecated.
| listen(port: number, hostname?: string, backlog?: number, listeningListener?: () => void): http.Server; | ||
| listen(port: number, hostname?: string, listeningListener?: () => void): http.Server; | ||
| listen(path: string, listeningListener?: () => void): http.Server; | ||
| listen(handle: any, listeningListener?: () => void): http.Server; |
There was a problem hiding this comment.
using listeningListener?: () => void instead of Function, to match signature of Server.listen
There was a problem hiding this comment.
I could have also added the other overloads of Server.listen (since it's the exact same method) but I wasn't sure if I should do that (I only looked at the latest version of Node) so I left only the ones already present.
| /** Register a middleware. */ | ||
| use(fn: HandleFunction): Server; | ||
| /** Mount a middleware on the specified route. */ | ||
| use(route: string, fn: HandleFunction): Server; |
There was a problem hiding this comment.
I originally intended to replace them with the overloads above, with the direct explicit parameters of fn callbacks visible, because that's more practical to handle in the editor (VSCode) after autocomplete, etc. Also, that allows more accurate documentation on each specific overload.
However, I had to keep the overloads with HandleFunction to avoid breaking compatibility in the tests of other packages, which referenced HandleFunction interface directly by name. Maybe I should have added @deprecated here as well.
| * `.createServer()` in Node.js. | ||
| */ | ||
| declare function createServer(): createServer.Server; | ||
| declare function createServer(): connect.Server; |
There was a problem hiding this comment.
When hovering const connect = require("connect"); the signature here shows the namespace as "connect" instead of "createServer" beacuse of the import trick above (line 11).
| export type NextHandleFunction = (req: http.IncomingMessage, res: http.ServerResponse, next: NextFunction) => void; | ||
| export type ErrorHandleFunction = (err: any, req: http.IncomingMessage, res: http.ServerResponse, next: NextFunction) => void; | ||
| export type HandleFunction = SimpleHandleFunction | NextHandleFunction | ErrorHandleFunction; | ||
| /** @deprecated `next` is always available to the handler */ export type SimpleHandleFunction = NextHandleFunction; | ||
| export type NextHandleFunction = | ||
| (req: Request | http.IncomingMessage, res: http.ServerResponse, next: NextFunction) => void; | ||
| export type ErrorHandleFunction = | ||
| (err: any, req: Request | http.IncomingMessage, res: http.ServerResponse, next: NextFunction) => void; | ||
| export type HandleFunction = NextHandleFunction | ErrorHandleFunction; |
There was a problem hiding this comment.
Param req was originally changed to be connect.Request (with originalUrl) but I had to keep http.IncomingMessage too to avoid breaking a test in another package which referenced the HandleFunction type directly by name (even though its a subtype).
| (req: http.IncomingMessage, res: http.ServerResponse, next?: Function): void; | ||
| /** | ||
| * The `app` itself is a function. This is just an alias to `app.handle`. | ||
| */ | ||
| (req: http.IncomingMessage, res: http.ServerResponse, next?: NextFunction): void; |
There was a problem hiding this comment.
This is the exact same as handle(req, res, out) according to code and API.
| * Handle server requests, punting them down | ||
| * the middleware stack. | ||
| * | ||
| * @private | ||
| */ | ||
| handle(req: http.IncomingMessage, res: http.ServerResponse, next: Function): void; | ||
| * Calling the function will run the middleware stack against the given Node.js http request (`req`) | ||
| * and response (`res`) objects. An optional function `out` can be provided that will be called if the | ||
| * request (or error) was not handled by the middleware stack. | ||
| */ | ||
| handle(req: http.IncomingMessage, res: http.ServerResponse, out?: NextFunction): void; |
There was a problem hiding this comment.
An optional function out can be provided that will be called if the request (or error) was not handled by the middleware stack.
Connect code (lines 127, 151) confirms that the out param is optional and called with err as parameter.
@EvanHahn I have added comments to clarify the most significant code changes. The rest is pretty much documentation (not included in the first commit). All code changes are already basically concentrated in the first commit, so it might be easier to review that one first. |
EvanHahn
left a comment
EvanHahn
left a comment
There was a problem hiding this comment.
Thanks for doing this.
I'd still prefer this to be multiple PRs for a few reasons:
- The commit message title includes some, but not all, of the things you're doing in this change. This is really a full rewrite of the package. For example, you've changed TSLint, cleaned up some of the test files, and changed the exported interface from
createServertoconnect. Too many things are happening to capture in the title of a single commit or PR. - Even with your helpful comments, it's still hard to review. There are lots of things I don't understand the intention behind, even if they're a good idea. For example, you added some comments to the top of the test file, but I don't know why.
- Someone going through the commit history in the future will also have similar issues breaking down the change. For example, someone trying to see what happened a year from now might not be able to figure out the intention behind some change.
- Large changes have a bigger chance of causing disruption to users of
@types/connect. If you made a mistake and I fail to catch it in review, we're going to cause a bunch of people a lot of trouble. If the changes are smaller, that could still happen, but the issues are localized.
| @@ -1,45 +1,65 @@ | |||
| // npm i --no-save connect | |||
There was a problem hiding this comment.
What's the intent behind these two comments? Are they needed?
|
|
||
| // Use legacy `Function` for `next` parameter. | ||
| app.use((req: http.IncomingMessage, res: http.ServerResponse, next: Function) => { | ||
| // tslint:disable-next-line ban-types |
There was a problem hiding this comment.
Why does this line need linting disabled?
| app.listen(3000); | ||
| // create node.js http server and listen on port using connect shortcut | ||
| app2.listen(3001); | ||
| console.log("http://localhost:3001/"); |
| app.use((err: any, req: http.IncomingMessage, res: http.ServerResponse, next: connect.NextFunction) => { | ||
| function stopOnErr(err: any, req: connect.Request, res: http.ServerResponse, next: connect.NextFunction) { | ||
| if (err) { | ||
| return res.end(`Error: ${err}`); |
There was a problem hiding this comment.
Why did this need to change?
| function stopOnErr(err: any, req: connect.Request, res: http.ServerResponse, next: connect.NextFunction) { | ||
| if (err) { | ||
| return res.end(`Error: ${err}`); | ||
| res.statusCode = +err || 500; |
There was a problem hiding this comment.
This +err thing feels weird. If we decide to keep these changes, could we only set the status code if typeof err === 'number'?
| const app = connect(); | ||
|
|
||
| // log all requests | ||
| app.use((req: http.IncomingMessage, res: http.ServerResponse, next: connect.NextFunction) => { |
There was a problem hiding this comment.
We're ripping out a lot of tests that ensure that req can be an IncomingMessage. I still think that should be valid, so could you make sure all of the cases are tested with both types?
| //create node.js http server and listen on port | ||
| app.use(stopOnErr); | ||
|
|
||
| // create node.js http server and listen on port |
There was a problem hiding this comment.
Fine to clean up this comment, but I think that should be done separately to shrink the size of these changes.
| @@ -1,94 +1,163 @@ | |||
| // Type definitions for connect v3.4.0 | |||
| // Type definitions for connect 3.7 | |||
There was a problem hiding this comment.
Should we leave an exact version number?
| * When the `connect` module is required, a function is returned that will construct a new app when called. | ||
| * This app will store all the middleware added and is, itself, a function. | ||
| * | ||
| * The core of Connect is "using" middleware. Middleware are added as a "stack" where incoming |
There was a problem hiding this comment.
The first two paragraphs seem fine but why is there so much here? Is this a typical amount of documentation?
| export type HandleFunction = SimpleHandleFunction | NextHandleFunction | ErrorHandleFunction; | ||
| /** @deprecated `next` is always available to the handler */ export type SimpleHandleFunction = NextHandleFunction; | ||
| export type NextHandleFunction = | ||
| (req: Request | http.IncomingMessage, res: http.ServerResponse, next: NextFunction) => void; |
There was a problem hiding this comment.
This may be a stupid question, but if Request is a subtype of IncomingMessage, can we just use Request here?
|
OK, I understand. Just one question though: should I break it into multiple pull requests or can I have a single pull request with all the commits of each individual change? I'm new to this (pull requests), so I don't know how it would work if you were going to, for example, approve some change and reject another. |
|
I think there are various pieces that could be done completely in parallel. For example, you could probably add support |
|
@geekley I haven't seen anything from you in a while and this PR currently has problems that prevent it from being merged. The PR will be closed tomorrow if there aren't new commits to fix the issues. |
typescript-bot
added
the
Abandoned
|
@geekley To keep things tidy, we have to close PRs that aren't mergeable but don't have activity from their author. No worries, though - please open a new PR if you'd like to continue with this change. Thank you! |
3 participants
Please fill in this template.
npm test.)npm run lint package-name(ortscif notslint.jsonis present).Select one of these and delete the others:
If changing an existing definition:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }. If for reason the any rule need to be disabled, disable it for that line using// tslint:disable-next-line [ruleName]and not for whole package so that the need for disabling can be reviewed.