[connect] Add missing property originalUrl to req parameter#40776
[connect] Add missing property originalUrl to req parameter#40776orta merged 1 commit intoDefinitelyTyped:masterfrom
Conversation
typescript-bot
added
Popular package
|
@trygveaa 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. |
|
👋 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? Comparison details 📊
It looks like nothing changed too much. I won’t post performance data again unless it gets worse. |
typescript-bot
added
the
Perf: Same
EvanHahn
left a comment
EvanHahn
left a comment
There was a problem hiding this comment.
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. |
typescript-bot
added
Revision needed
|
@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
trygveaa
force-pushed
the
connect-req-add-originalUrl
branch
from
3e3533e to
fe6e40c
Compare
|
@EvanHahn: I've added the test. |
|
🔔 @EvanHahn - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate? |
| export type ServerHandle = HandleFunction | http.Server; | ||
|
|
||
| export class IncomingMessage extends http.IncomingMessage { | ||
| originalUrl?: http.IncomingMessage["url"]; |
There was a problem hiding this comment.
I'm not a TypeScript expert. What does this line do?
There was a problem hiding this comment.
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.
typescript-bot
added
Owner Approved
|
A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped! |
|
Looks good and thanks for the test to verify existing code doesn't break 👍 |
|
I just published |
| declare namespace createServer { | ||
| export type ServerHandle = HandleFunction | http.Server; | ||
|
|
||
| export class IncomingMessage extends http.IncomingMessage { |
There was a problem hiding this comment.
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.
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.
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
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
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
5 participants
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
originalUrlis 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(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.