[Node]: Update http request listeners by vansergen · Pull Request #61922 · DefinitelyTyped/DefinitelyTyped

vansergen · 2022-08-24T16:31:51Z

Description

The second attempt to merge #58879.

Motivation

import { IncomingMessage, ServerResponse, createServer } from "node:http";
class CustomIncomingMessage extends IncomingMessage {}
class CustomServerResponse extends ServerResponse {}
createServer({
  IncomingMessage: CustomIncomingMessage,
  ServerResponse: CustomServerResponse,
})
  .on("upgrade", (req) => {
    // $ExpectType CustomIncomingMessage
    req;
  })
  .on("request", (req, res) => {
    // $ExpectType CustomIncomingMessage
    req;
    // $ExpectType CustomServerResponse
    res;
    // $ExpectType CustomIncomingMessage
    res.req;
  });

Checklist

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.
Follow the advice from the readme.
Avoid common mistakes.
Run npm test <package to test>.

…tom-request-response

Pass classes directly to generic types Ref: 38c1f13

typescript-bot · 2022-08-24T16:32:39Z

@vansergen Thank you for submitting this PR!

This is a live comment which I will keep updated.

4 packages in this PR

node — on npm, on unpkg
- owner-approval: @mcollina
on-finished — on npm, on unpkg
request-stats — on npm, on unpkg
stoppable — on npm, on unpkg

Code Reviews

This PR can be merged once it's reviewed.

You can test the changes of this PR in the Playground.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
✅ Type definition owners or DT maintainers needs to approve changes which affect more than one package

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": 61922,
  "author": "vansergen",
  "headCommitOid": "302f650c7986b4efac2a2e158b13ee286ffa351e",
  "mergeBaseOid": "5c248b832935aebece0abff07360102f1e9111f3",
  "lastPushDate": "2022-09-07T09:39:48.000Z",
  "lastActivityDate": "2022-09-07T20:22:09.000Z",
  "maintainerBlessed": "Waiting for Code Reviews",
  "mergeOfferDate": "2022-09-07T18:59:48.000Z",
  "mergeRequestDate": "2022-09-07T20:22:09.000Z",
  "mergeRequestUser": "vansergen",
  "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/https.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/node-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/node/test/http.ts",
          "kind": "test"
        },
        {
          "path": "types/node/test/https.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v14/http.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v14/https.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v14/test/http.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v14/test/https.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v16/http.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/https.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/node-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v16/test/http.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v16/test/https.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "Microsoft",
        "DefinitelyTyped",
        "jkomyno",
        "alvis",
        "r3nya",
        "btoueg",
        "smac89",
        "touffy",
        "DeividasBakanas",
        "eyqs",
        "Hannes-Magnusson-CK",
        "hoo29",
        "kjin",
        "ajafff",
        "islishude",
        "mwiktorczyk",
        "mohsen1",
        "n-e",
        "galkin",
        "parambirs",
        "eps1lon",
        "SimonSchick",
        "ThomasdenH",
        "WilcoBakker",
        "wwwy3y3",
        "samuela",
        "kuehlein",
        "bhongy",
        "chyzwar",
        "trivikr",
        "yoursunny",
        "qwelias",
        "ExE-Boss",
        "peterblazejewicz",
        "addaleax",
        "victorperin",
        "ZYSzys",
        "NodeJS",
        "LinusU",
        "wafuwafu13",
        "mcollina"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    },
    {
      "name": "on-finished",
      "kind": "edit",
      "files": [
        {
          "path": "types/on-finished/on-finished-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "czechboy0",
        "BendingBender"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    },
    {
      "name": "request-stats",
      "kind": "edit",
      "files": [
        {
          "path": "types/request-stats/request-stats-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "BendingBender"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "stoppable",
      "kind": "edit",
      "files": [
        {
          "path": "types/stoppable/stoppable-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "EricByers",
        "jplusje",
        "BendingBender"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "mcollina",
      "date": "2022-09-07T20:17:20.000Z",
      "isMaintainer": false
    },
    {
      "type": "approved",
      "reviewer": "sheetalkamat",
      "date": "2022-09-07T18:59:01.000Z",
      "isMaintainer": true
    }
  ],
  "mainBotCommentID": 1225963261,
  "ciResult": "pass"
}

typescript-bot · 2022-08-24T16:32:43Z

🔔 @microsoft @DefinitelyTyped @jkomyno @alvis @r3nya @btoueg @smac89 @Touffy @DeividasBakanas @eyqs @Hannes-Magnusson-CK @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @n-e @galkin @parambirs @eps1lon @SimonSchick @ThomasdenH @WilcoBakker @wwwy3y3 @samuela @kuehlein @bhongy @chyzwar @trivikr @yoursunny @qwelias @ExE-Boss @peterblazejewicz @addaleax @victorperin @ZYSzys @nodejs @LinusU @wafuwafu13 @mcollina @czechboy0 @BendingBender @EricByers @jplusje — 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.

mcollina

lgtm

mcollina

I fear this would be too breaking as a change.

typescript-bot · 2022-08-24T16:40:16Z

@vansergen 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. Thank you!

vansergen · 2022-08-24T16:56:34Z

I fear this would be too breaking as a change.

I believe it could be a breaking change in tests (like it happened in other packages on-finished, stoppable etc.) when $ExpectType is used

typescript-bot · 2022-08-24T17:38:52Z

@mcollina Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

typescript-bot · 2022-09-05T12:02:15Z

@vansergen 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 which are failing do not end up on the list of PRs for the DT maintainers to review.

sheetalkamat

Need maintainer signoff to see how is the new approach.

typescript-bot · 2022-09-06T23:32:48Z

@vansergen 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. Thank you!

mcollina

lgtm

vansergen · 2022-09-07T09:32:37Z

@sheetalkamat Could you please be more specific on requested changes?

typescript-bot · 2022-09-07T10:13:38Z

@mcollina, @sheetalkamat Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

typescript-bot · 2022-09-07T18:59:49Z

@mcollina Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

mcollina

lgtm

vansergen · 2022-09-07T20:22:09Z

Ready to merge

srajiang · 2022-09-08T20:24:21Z

Hello @vansergen @mcollina @sheetalkamat. I see this was just released as part of v18.7.16 - I posted a more fleshed out issue that we're seeing now using this version with these changes included.

Here's that issue:
#62170

It's breaking in our CI/CD atm, and reverting to a version without the changes resolves the issue.

From reading the error message, the changes have made the https and http createServer types incompatible.

mcollina · 2022-09-09T13:59:07Z

@vansergen could you revert this? I think it's breaking everybody.

vansergen · 2022-09-09T17:41:04Z

@mcollina Sure, I could create a new PR that reverses it. But I think we should collect those breaking examples first.

vansergen · 2022-09-10T09:51:23Z

The following

import * as http from "node:http";
import * as https from "node:https";
const createServerFn: typeof http.createServer | typeof https.createServer = https.createServer;
createServerFn({});
// TS2349: This expression is not callable. Each member of the union type has signatures, but none of those signatures are compatible with each other.

does not work now.

vansergen and others added 13 commits February 20, 2022 20:40

feat(http): add custom IncomingMessage and ServerResponse support

38c1f13

test(http): add custom IncomingMessage and ServerResponse

1749b3f

style: add empty spaces

7662362

test(node): add res.req

6164645

test(node): add res.req

3ac12bb

test: update request/response types

3777929

test(node): add createServer

5c11a8e

Merge pull request DefinitelyTyped#4 from b2broker/master

52c710f

Merge branch 'b2broker/http_request_response' into vansergen/node/cus…

cc42d07

…tom-request-response

feat(node): add custom IncomingMessage and ServerResponse support

2538f02

Pass classes directly to generic types Ref: 38c1f13

test: update interfaces

9e758b4

test(stoppable): update http types

31dd0c8

test(on-finished): update http types

49c4982

vansergen requested a review from peterblazejewicz as a code owner August 24, 2022 16:31

typescript-bot added Critical package Edits multiple packages labels Aug 24, 2022

mcollina approved these changes Aug 24, 2022

View reviewed changes

mcollina suggested changes Aug 24, 2022

View reviewed changes

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Aug 24, 2022

typescript-bot removed the Revision needed This PR needs code changes before it can be merged. label Aug 24, 2022

fix(node): update generic types

3c714aa

vansergen force-pushed the node/custom-request-response branch from 2d86275 to 3c714aa Compare August 24, 2022 16:48

Merge branch 'DefinitelyTyped:master' into node/custom-request-response

a19359f

vansergen added 2 commits August 25, 2022 14:59

fix(node:https): update ServerOptions

553a07d

Merge branch 'DefinitelyTyped:master' into node/custom-request-response

6995d01

Merge branch 'master' into node/custom-request-response

d35df07

typescript-bot added the The CI failed When GH Actions fails label Sep 5, 2022

vansergen force-pushed the node/custom-request-response branch from 84d3ca9 to d35df07 Compare September 5, 2022 12:02

typescript-bot removed New Definition This PR creates a new definition package. The CI failed When GH Actions fails labels Sep 5, 2022

sheetalkamat requested changes Sep 6, 2022

View reviewed changes

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Sep 6, 2022

mcollina approved these changes Sep 7, 2022

View reviewed changes

vansergen requested review from sheetalkamat and removed request for peterblazejewicz September 7, 2022 09:31

Merge branch 'DefinitelyTyped:master' into node/custom-request-response

302f650

typescript-bot removed the Revision needed This PR needs code changes before it can be merged. label Sep 7, 2022

sheetalkamat approved these changes Sep 7, 2022

View reviewed changes

typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner labels Sep 7, 2022

mcollina approved these changes Sep 7, 2022

View reviewed changes

typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Sep 7, 2022

typescript-bot merged commit 05b8eef into DefinitelyTyped:master Sep 7, 2022

vansergen deleted the node/custom-request-response branch September 8, 2022 18:13

vansergen mentioned this pull request May 15, 2023

[ws]: Add generics to the ServerOptions #65501

Merged

8 tasks

Conversation

vansergen commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation

Checklist

Uh oh!

typescript-bot commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

4 packages in this PR

Code Reviews

Status

Uh oh!

typescript-bot commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Aug 24, 2022

Uh oh!

vansergen commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Aug 24, 2022

Uh oh!

typescript-bot commented Sep 5, 2022

Uh oh!

sheetalkamat left a comment

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Sep 6, 2022

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

vansergen commented Sep 7, 2022

Uh oh!

typescript-bot commented Sep 7, 2022

Uh oh!

typescript-bot commented Sep 7, 2022

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

vansergen commented Sep 7, 2022

Uh oh!

srajiang commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Sep 9, 2022

Uh oh!

vansergen commented Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vansergen commented Sep 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vansergen commented Aug 24, 2022 •

edited

Loading

typescript-bot commented Aug 24, 2022 •

edited

Loading

typescript-bot commented Aug 24, 2022 •

edited

Loading

vansergen commented Aug 24, 2022 •

edited

Loading

srajiang commented Sep 8, 2022 •

edited

Loading

vansergen commented Sep 9, 2022 •

edited

Loading

vansergen commented Sep 10, 2022 •

edited

Loading