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 missing property originalUrl to req parameter #40776
[connect] Add missing property originalUrl to req parameter #40776
Conversation
@trygveaa Thank you for submitting this PR!
If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
Let’s review the numbers, shall we? Comparison details
|
master | #40776 | diff | |
---|---|---|---|
Batch compilation | |||
Memory usage (MiB) | 63.6 | 63.5 | -0.2% |
Type count | 8398 | 8419 | 0% |
Assignability cache size | 901 | 903 | 0% |
Language service | |||
Samples taken | 89 | 89 | 0% |
Identifiers in tests | 89 | 89 | 0% |
getCompletionsAtPosition |
|||
Mean duration (ms) | 308.4 | 308.6 | 0.0% |
Mean CV | 11.8% | 10.9% | |
Worst duration (ms) | 439.9 | 388.8 | -11.6% |
Worst identifier | http | app | |
getQuickInfoAtPosition |
|||
Mean duration (ms) | 305.8 | 309.8 | +1.3% |
Mean CV | 10.4% | 11.9% | +14.7% |
Worst duration (ms) | 361.7 | 371.3 | +2.7% |
Worst identifier | IncomingMessage | res |
It looks like nothing changed too much. I won’t post performance data again unless it gets worse.
Rejecting because I have a question:
Should we test that a request handler still works with http.IncomingMessage
? For example, should we have a test like this?
app.use((req: http.IncomingMessage, res: http.ServerResponse) => {
console.log(req, res);
res.end();
});
@EvanHahn: Not sure if it's a question for me, or another maintainer. But sure, I can add that test if you want. I agree that it's a good idea. |
@trygveaa 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! |
This adds a new class IncomingMessage which extends from http.IncomingMessage and adds the originalUrl property which was previously missing. This new class is used for the req parameter in HandleFunction. The originalUrl property is added to req here: https://github.com/senchalabs/connect/blob/3.4.0/index.js#L133
3e3533e
to
fe6e40c
Compare
@EvanHahn: I've added the test. |
|
@@ -18,11 +18,15 @@ declare function createServer(): createServer.Server; | |||
declare namespace createServer { | |||
export type ServerHandle = HandleFunction | http.Server; | |||
|
|||
export class IncomingMessage extends http.IncomingMessage { | |||
originalUrl?: http.IncomingMessage["url"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a TypeScript expert. What does this line do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http.IncomingMessage["url"]
is a reference to the type of the url
property in http.IncomingMessage
, so it's setting the type of originalUrl
to be the same as the type of url
. It's called lookup types, documented here.
A definition owner has approved this PR |
Looks good and thanks for the test to verify existing code doesn't break |
I just published |
@@ -18,11 +18,15 @@ declare function createServer(): createServer.Server; | |||
declare namespace createServer { | |||
export type ServerHandle = HandleFunction | http.Server; | |||
|
|||
export class IncomingMessage extends http.IncomingMessage { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be "implements" as http.IncomingMessage is an interface.
My code will no longer compile in typescript 3.7.2 because of this issue.
Full error is "node_modules/@types/connect/index.d.ts(21,42): error TS2689: Cannot extend an in
terface 'http.IncomingMessage'. Did you mean 'implements'?"
Please can we fix this ASAP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the types in this repo, http.IncomingMessage
is a class. Where are you getting your types from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks for that. Turns out we were using an older version of node @types where the http.IncomingMessage
was an interface. Thanks for your prompt response
connect now has it's own IncomingMessage class which inherits from http and includes the originalUrl property. The request instances here are coming from connect, so they should use this class. The IncomingMessage class from connect was added in DefinitelyTyped/DefinitelyTyped#40776
connect now has it's own IncomingMessage class which inherits from http and includes the originalUrl property. The request instances here are coming from connect, so they should use this class. The IncomingMessage class from connect was added in DefinitelyTyped/DefinitelyTyped#40776
connect now has it's own IncomingMessage class which inherits from http and includes the originalUrl property. The request instances here are coming from connect, so they should use this class. The IncomingMessage class from connect was added in DefinitelyTyped/DefinitelyTyped#40776
This adds a new class IncomingMessage which extends from
http.IncomingMessage and adds the originalUrl property which was
previously missing. This new class is used for the req parameter in
HandleFunction.
The originalUrl property is added to req here:
https://github.com/senchalabs/connect/blob/3.4.0/index.js#L133
originalUrl
is present in the version specified in these type definitions, so I didn't change the version.Please fill in this template.
npm test
.)npm run lint package-name
(ortsc
if notslint.json
is present).Select one of these and delete the others:
If changing an existing definition:
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.