Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

connect: add req.originalUrl; improve API; API docs from README #40409

Closed
wants to merge 3 commits into from

Conversation

@geekley
Copy link

geekley commented Nov 15, 2019

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: https://www.npmjs.com/package/connect#appuseroute-fn
  • If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
  • If you are making substantial changes, consider adding a tslint.json containing { "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.
@typescript-bot

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Nov 15, 2019

👋 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?

master #40409 diff
Batch compilation
Memory usage (MiB) 64.6 65.3 +1.0%
Type count 8386 8474 +1%
Assignability cache size 903 904 0%
Language service
Samples taken 89 112 +26%
Identifiers in tests 89 112 +26%
getCompletionsAtPosition
    Mean duration (ms) 304.2 334.5 +10.0%
    Mean CV 12.7% 12.4%
    Worst duration (ms) 374.8 477.6 +27.4% 🔸
    Worst identifier http use
getQuickInfoAtPosition
    Mean duration (ms) 304.1 331.9 +9.1%
    Mean CV 12.7% 11.1% -12.1%
    Worst duration (ms) 379.6 402.1 +5.9%
    Worst identifier use createServer

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

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Nov 15, 2019

@geekley Thank you for submitting this PR!

🔔 @SomaticIT @EvanHahn - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

Copy link
Contributor

EvanHahn left a comment

IMO, too many things are happening in this pull request. It seems like there are a few distinct pieces:

  1. Add documentation
  2. Add connect.Request, which extends http.IncomingMessage with an originalUrl
  3. 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;

This comment has been minimized.

Copy link
@EvanHahn

EvanHahn Nov 16, 2019

Contributor

This is already a property of http.IncomingMessage. Have you added this for documentation purposes?

This comment has been minimized.

Copy link
@geekley

geekley Nov 16, 2019

Author

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 typescript-bot moved this from Waiting for Reviewers to Needs Author Attention in Pull Request Status Board Nov 16, 2019
@typescript-bot

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Nov 16, 2019

@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!


export type SimpleHandleFunction = (req: http.IncomingMessage, res: http.ServerResponse) => void;

This comment has been minimized.

Copy link
@geekley

geekley Nov 16, 2019

Author

"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;
Comment on lines +156 to +159

This comment has been minimized.

Copy link
@geekley

geekley Nov 16, 2019

Author

using listeningListener?: () => void instead of Function, to match signature of Server.listen

This comment has been minimized.

Copy link
@geekley

geekley Nov 17, 2019

Author

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;
Comment on lines +137 to 140

This comment has been minimized.

Copy link
@geekley

geekley Nov 16, 2019

Author

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.

*/
declare function createServer(): createServer.Server;
declare function createServer(): connect.Server;

This comment has been minimized.

Copy link
@geekley

geekley Nov 16, 2019

Author

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;
Comment on lines -24 to +57

This comment has been minimized.

Copy link
@geekley

geekley Nov 16, 2019

Author

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;
Comment on lines -34 to +68

This comment has been minimized.

Copy link
@geekley

geekley Nov 16, 2019

Author

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;
Comment on lines -56 to +147

This comment has been minimized.

Copy link
@geekley

geekley Nov 16, 2019

Author

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.

@geekley

This comment has been minimized.

Copy link
Author

geekley commented Nov 17, 2019

IMO, too many things are happening in this pull request. It seems like there are a few distinct pieces

@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.

Copy link
Contributor

EvanHahn left a comment

Thanks for doing this.

I'd still prefer this to be multiple PRs for a few reasons:

  1. 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 createServer to connect. Too many things are happening to capture in the title of a single commit or PR.
  2. 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.
  3. 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.
  4. 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

This comment has been minimized.

Copy link
@EvanHahn

EvanHahn Nov 17, 2019

Contributor

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

This comment has been minimized.

Copy link
@EvanHahn

EvanHahn Nov 17, 2019

Contributor

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/");

This comment has been minimized.

Copy link
@EvanHahn

EvanHahn Nov 17, 2019

Contributor

Why is this lot needed?

if (err) {
return res.end(`Error: ${err}`);

This comment has been minimized.

Copy link
@EvanHahn

EvanHahn Nov 17, 2019

Contributor

Why did this need to change?

if (err) {
return res.end(`Error: ${err}`);
res.statusCode = +err || 500;

This comment has been minimized.

Copy link
@EvanHahn

EvanHahn Nov 17, 2019

Contributor

This +err thing feels weird. If we decide to keep these changes, could we only set the status code if typeof err === 'number'?

import * as http from "http";
import connect = require("connect");

const app = connect();

// log all requests
app.use((req: http.IncomingMessage, res: http.ServerResponse, next: connect.NextFunction) => {

This comment has been minimized.

Copy link
@EvanHahn

EvanHahn Nov 17, 2019

Contributor

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

This comment has been minimized.

Copy link
@EvanHahn

EvanHahn Nov 17, 2019

Contributor

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

This comment has been minimized.

Copy link
@EvanHahn

EvanHahn Nov 17, 2019

Contributor

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

This comment has been minimized.

Copy link
@EvanHahn

EvanHahn Nov 17, 2019

Contributor

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;

This comment has been minimized.

Copy link
@EvanHahn

EvanHahn Nov 17, 2019

Contributor

This may be a stupid question, but if Request is a subtype of IncomingMessage, can we just use Request here?

@geekley

This comment has been minimized.

Copy link
Author

geekley commented Nov 17, 2019

OK, I understand.
I'm going to respond to your comments later and after that I'll redo the commits with a more step-by-step approach as you said.

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.
What I mean is: are you able to approve only up to a certain commit in the pull request, or are you only able to approve/reject all of it? Can you approve specific blocks of code only (like staging)?
If you need to edit code, does that create more commits in the pull request, or is it "just changed"?
Also, should I use the same branch for everything?

@EvanHahn

This comment has been minimized.

Copy link
Contributor

EvanHahn commented Nov 17, 2019

I think there are various pieces that could be done completely in parallel. For example, you could probably add support req.originalUrl all on its own; you could change createServer to connect all on its own; you could update the TSLint file all on its own. Some of the changes might depend on others, but I think it'd be useful to try to break them down into the smallest possible pieces.

@typescript-bot

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Nov 23, 2019

@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

This comment has been minimized.

Copy link
Contributor

typescript-bot commented Nov 24, 2019

@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!

Pull Request Status Board automation moved this from Needs Author Attention to Done Nov 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.