[node] Fix http.createServer signature (again) #70455
[node] Fix http.createServer signature (again) #70455typescript-bot merged 15 commits intoDefinitelyTyped:masterfrom
http.createServer signature (again) #70455Conversation
Usual caseimport * as http from "node:http"
export const createServer = () => {
return http.createServer();
}Emits declaration: import * as http from "node:http";
export declare const createServer: () => http.Server<typeof http.IncomingMessage, typeof http.ServerResponse>;Case with custom prototypesimport * as http from "node:http"
class Request extends http.IncomingMessage {}
class Response<Req extends Request> extends http.ServerResponse<Req> {}
export const createServer = () => {
return http.createServer<typeof Request, typeof Response>();
}Emits declaration: import * as http from "node:http";
declare class Request extends http.IncomingMessage {}
declare class Response<Req extends Request> extends http.ServerResponse<Req> {}
export declare const createServer: () => http.Server<typeof Request, typeof Response>;
export {}; |
|
@andrewbranch If you have time, I would really appreciate your review on this ❤️ |
http.createServer (again) http.createServer signature (again)
|
First check failure is a test I need to adjust, as the type being asserted upon was previously not generic. Second check failure looks to be a problem on |
|
@jakebailey I'm afraid you're my only port of call for ecosystem breaks 😅 please do let me know if you'd prefer I not ping you like this |
|
@Lordfirespeed Thank you for submitting this PR! This is a live comment that I will keep updated. 1 package in this PRCode ReviewsBecause this is a widely-used package, a DT maintainer will need to review it before it can be merged. You can test the changes of this PR in the Playground. Status
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 70455,
"author": "Lordfirespeed",
"headCommitOid": "35cd007c18de6b6cca94d0ea37483f69b99f1eff",
"mergeBaseOid": "7a5210ab6680efcb9d2aba9128ce4418974e9638",
"lastPushDate": "2024-09-05T04:03:59.000Z",
"lastActivityDate": "2024-09-25T00:21:57.000Z",
"mergeOfferDate": "2024-09-25T00:20:40.000Z",
"mergeRequestDate": "2024-09-25T00:21:57.000Z",
"mergeRequestUser": "Lordfirespeed",
"hasMergeConflict": false,
"isFirstContribution": false,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Critical",
"pkgInfo": [
{
"name": "node",
"kind": "edit",
"files": [
{
"path": "types/node/http.d.ts",
"kind": "definition"
},
{
"path": "types/node/http2.d.ts",
"kind": "definition"
},
{
"path": "types/node/https.d.ts",
"kind": "definition"
},
{
"path": "types/node/test/http.ts",
"kind": "test"
},
{
"path": "types/node/test/http2.ts",
"kind": "test"
},
{
"path": "types/node/v16/http.d.ts",
"kind": "definition"
},
{
"path": "types/node/v16/http2.d.ts",
"kind": "definition"
},
{
"path": "types/node/v16/https.d.ts",
"kind": "definition"
},
{
"path": "types/node/v16/test/http2.ts",
"kind": "test"
},
{
"path": "types/node/v18/http.d.ts",
"kind": "definition"
},
{
"path": "types/node/v18/http2.d.ts",
"kind": "definition"
},
{
"path": "types/node/v18/https.d.ts",
"kind": "definition"
},
{
"path": "types/node/v18/test/http2.ts",
"kind": "test"
},
{
"path": "types/node/v20/http.d.ts",
"kind": "definition"
},
{
"path": "types/node/v20/http2.d.ts",
"kind": "definition"
},
{
"path": "types/node/v20/https.d.ts",
"kind": "definition"
},
{
"path": "types/node/v20/test/http2.ts",
"kind": "test"
}
],
"owners": [
"Microsoft",
"jkomyno",
"alvis",
"r3nya",
"btoueg",
"smac89",
"touffy",
"DeividasBakanas",
"eyqs",
"Hannes-Magnusson-CK",
"hoo29",
"kjin",
"ajafff",
"islishude",
"mwiktorczyk",
"mohsen1",
"galkin",
"parambirs",
"eps1lon",
"ThomasdenH",
"WilcoBakker",
"wwwy3y3",
"samuela",
"kuehlein",
"bhongy",
"chyzwar",
"trivikr",
"yoursunny",
"qwelias",
"ExE-Boss",
"peterblazejewicz",
"addaleax",
"victorperin",
"ZYSzys",
"NodeJS",
"LinusU",
"wafuwafu13",
"mcollina",
"Semigradsky"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Critical"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "jakebailey",
"date": "2024-09-25T00:16:59.000Z",
"isMaintainer": true
}
],
"mainBotCommentID": 2331574541,
"ciResult": "pass"
} |
|
🔔 @microsoft @jkomyno @alvis @r3nya @btoueg @smac89 @Touffy @DeividasBakanas @eyqs @Hannes-Magnusson-CK @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @galkin @parambirs @eps1lon @ThomasdenH @WilcoBakker @wwwy3y3 @samuela @kuehlein @bhongy @chyzwar @trivikr @yoursunny @qwelias @ExE-Boss @peterblazejewicz @addaleax @victorperin @ZYSzys @nodejs @LinusU @wafuwafu13 @mcollina @Semigradsky — please review this PR in the next few days. Be sure to explicitly select |
|
@Lordfirespeed The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! Note: builds that are failing do not end up on the list of PRs for the DT maintainers to review. |
|
Found the ecosystem break: Automattic/mongoose#14863 |
|
Re-ping @microsoft, @jkomyno, @alvis, @r3nya, @btoueg, @smac89, @Touffy, @DeividasBakanas, @eyqs, @Hannes-Magnusson-CK, @hoo29, @kjin, @ajafff, @islishude, @mwiktorczyk, @mohsen1, @galkin, @parambirs, @eps1lon, @ThomasdenH, @WilcoBakker, @wwwy3y3, @samuela, @kuehlein, @bhongy, @chyzwar, @trivikr, @yoursunny, @qwelias, @ExE-Boss, @peterblazejewicz, @addaleax, @victorperin, @ZYSzys, @nodejs, @LinusU, @wafuwafu13, @mcollina, @Semigradsky: This PR has been out for over a week, yet I haven't seen any reviews. Could someone please give it some attention? Thanks! |
|
It has been more than two weeks and this PR still has no reviews. I'll bump it to the DT maintainer queue. Thank you for your patience, @Lordfirespeed. (Ping @microsoft, @jkomyno, @alvis, @r3nya, @btoueg, @smac89, @Touffy, @DeividasBakanas, @eyqs, @Hannes-Magnusson-CK, @hoo29, @kjin, @ajafff, @islishude, @mwiktorczyk, @mohsen1, @galkin, @parambirs, @eps1lon, @ThomasdenH, @WilcoBakker, @wwwy3y3, @samuela, @kuehlein, @bhongy, @chyzwar, @trivikr, @yoursunny, @qwelias, @ExE-Boss, @peterblazejewicz, @addaleax, @victorperin, @ZYSzys, @nodejs, @LinusU, @wafuwafu13, @mcollina, @Semigradsky.) |
|
Is mongoose guaranteed to not break this time? |
Mongoose was never breaking because of this PR's (or the previous one's) changes 😅 That said, I have very high confidence they are not broken by this PR, because it does not change the generic type parameter defaults, just the constraints applied to the generic type parameters when supplied. So
The only potential breaking change I envision is one where someone is supplying only one generic type parameter to const server = http.createServer<typeof MyRequest>(...)But we can test that this is not an issue: class Foo {}
class Bar<T extends Foo = Foo> {}
function createServer<
TFoo extends typeof Foo = typeof Foo,
TBar extends typeof Bar<TFoo> = typeof Bar,
>(listener: (foo: TFoo, bar: TBar) => void) {}
class MyFoo extends Foo {}
createServer<typeof MyFoo>((foo, bar) => {}) |
|
I retested Andrew's example from #70289 (comment), and the dts emit is now the same before/after this PR, so that issue is also fixed. |
jakebailey
left a comment
There was a problem hiding this comment.
Willing to try this a second time given the extra testing we did.
|
Er, well, you may want to merge master and just verify that things work. |
|
@Lordfirespeed: Everything looks good here. I am ready to merge this PR (at 35cd007) on your behalf whenever you think it's ready. If you'd like that to happen, please post a comment saying:
and I'll merge this PR almost instantly. Thanks for helping out! ❤️ (@microsoft, @jkomyno, @alvis, @r3nya, @btoueg, @smac89, @Touffy, @DeividasBakanas, @eyqs, @Hannes-Magnusson-CK, @hoo29, @kjin, @ajafff, @islishude, @mwiktorczyk, @mohsen1, @galkin, @parambirs, @eps1lon, @ThomasdenH, @WilcoBakker, @wwwy3y3, @samuela, @kuehlein, @bhongy, @chyzwar, @trivikr, @yoursunny, @qwelias, @ExE-Boss, @peterblazejewicz, @addaleax, @victorperin, @ZYSzys, @nodejs, @LinusU, @wafuwafu13, @mcollina, @Semigradsky: you can do this too.) |
|
Ready to merge |
…ture (again) by @Lordfirespeed
pnpm test <package to test>.I am changing an existing definition
http.createServersignature #70289http.createServersignature" #70453This contains all changes from #70289 with the difference that the defaults for the
Responsetype parameters are left unchanged.This should ensure no existing code is broken.