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

vansergen · 2022-02-20T18:00:48Z

Motivation

import { IncomingMessage, ServerResponse, createServer } from "node:http";
class CustomIncomingMessage extends IncomingMessage {}
class CustomServerResponse extends ServerResponse<CustomIncomingMessage> {}
createServer<CustomIncomingMessage, CustomServerResponse>({
  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>.

typescript-bot · 2022-02-20T18:01:26Z

@vansergen Thank you for submitting this PR!

This is a live comment which I will keep updated.

5 packages in this PR

cacheable-request — on npm, on unpkg
node — on npm, on unpkg
on-finished — on npm, on unpkg
request-stats — on npm, on unpkg
stoppable — on npm, on unpkg

Code Reviews

Because 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

✅ No merge conflicts
❌ Continuous integration tests have failed
🕐 A DT maintainer needs to approve changes which affect more than one package

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Inactive

This PR has been inactive for 31 days — it is considered abandoned, and therefore closed!

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 58879,
  "author": "vansergen",
  "headCommitOid": "52c710ff8f5f2fb85d19cb793a1b52d519c5552f",
  "mergeBaseOid": "3265453bdd2a7d575ea6f2d43c34f644c0e496e0",
  "lastPushDate": "2022-04-04T10:37:06.000Z",
  "lastActivityDate": "2022-04-04T12:03:35.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "cacheable-request",
      "kind": "edit",
      "files": [
        {
          "path": "types/cacheable-request/cacheable-request-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "BendingBender",
        "paulmelnikow"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    },
    {
      "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/v12/http.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v12/https.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v12/test/http.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v12/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"
      ],
      "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": "stale",
      "reviewer": "amcasey",
      "date": "2022-03-21T21:43:34.000Z",
      "abbrOid": "5c11a8e"
    }
  ],
  "mainBotCommentID": 1046290458,
  "ciResult": "fail",
  "ciUrl": "https://github.com/DefinitelyTyped/DefinitelyTyped/commit/52c710ff8f5f2fb85d19cb793a1b52d519c5552f/checks?check_suite_id=5920276808"
}

typescript-bot · 2022-02-20T18:01:28Z

🔔 @BendingBender @paulmelnikow @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 @czechboy0 @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.

vansergen · 2022-02-26T14:41:43Z

Did anyone have a chance to look at it?

SimonSchick · 2022-02-26T15:27:05Z

The problem of this change is generally giving a false sense of type safety, asserting types through generics is generally considered an anti-pattern as it tends to hide the actual type assertions.

Then again any big library does this (including express, axios, etc. etc), I'm a little torn about doing it in node-core and would like other maintainers input in this.

vansergen · 2022-02-28T19:55:43Z

The problem of this change is generally giving a false sense of type safety, asserting types through generics is generally considered an anti-pattern as it tends to hide the actual type assertions.

Indeed, this PR does not solve any issues but gives a tool for type safety when http.createServer() is used with custom IncomingMessage/ServerResponse classes. I believe that such behavior is desired and will be useful (generics are used just once with http.createServer() or new Server())

createServer(
  {
    IncomingMessage: CustomIncomingMessage,
    ServerResponse: CustomServerResponse,
  },
  (req, res) => {
    assert.ok(req instanceof CustomIncomingMessage);
    assert.ok(res instanceof CustomServerResponse);
  }
);

Another problem is res.req - it depends on the classes were used with http.createServer().

class IncomingMessage1 extends IncomingMessage {}
class IncomingMessage2 extends IncomingMessage {}
createServer({ IncomingMessage: IncomingMessage1 }, (_req, res) => {
  res.req; // $ExpectType IncomingMessage1
});
createServer({ IncomingMessage: IncomingMessage2 }, (_req, res) => {
  res.req; // $ExpectType IncomingMessage2
});

It seems that server creation is the right place to define types that will be used in all listeners. This PR solves those problems without any breaking changes.

any big library does this (including express, axios, etc. etc)

It could mean that such functionality would be useful because all of them are based on the node:http module.

LinusU · 2022-03-01T09:51:11Z

Then again any big library does this (including express, axios, etc. etc)

I don't think Express does this?

I think that since you can actually pass the IncomingMessage and ServerResponse classes to createServer I think that we might want to support this. I am however afraid that this will inadvertently be used to hide type assertions and to create unsound code...

Maybe we could type createServer something like this?

function createServer(options?: Options): Server<IncomingMessage, ServerResponse<IncomingMessage>>
function createServer<T extends IncomingMessage>(options: Options & { IncomingMessage: typeof T }): Server<T, ServerResponse<T>>
function createServer<T extends ServerResponse<IncomingMessage>>(options: Options & { ServerResponse: typeof T }): Server<IncomingMessage, T>
function createServer<T extends IncomingMessage, U extends ServerResponse<T>>(options: Options & { IncomingMessage: typeof T, ServerResponse: typeof U }): Server<T, U>

That would save you having to manually type out the generic parameters when passing custom classes, and it would also ensure that you are actually passing the classes if you define the generic parameters.

typescript-bot · 2022-03-04T02:08:37Z

Re-ping @BendingBender, @paulmelnikow, @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, @czechboy0, @EricByers, @jplusje:

This PR has been out for over a week, yet I haven't seen any reviews.

Could someone please give it some attention? Thanks!

RyanCavanaugh · 2022-03-10T05:23:18Z

The problem of this change is generally giving a false sense of type safety, asserting types through generics is generally considered an anti-pattern as it tends to hide the actual type assertions.

Speaking from TS's perspective, I agree. I'd consider this a pattern of last resort. @LinusU's suggestion seemed promising

typescript-bot · 2022-03-11T02:06:29Z

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

(Ping @BendingBender, @paulmelnikow, @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, @czechboy0, @EricByers, @jplusje.)

amcasey

My understanding from the discussion above is that there's still work to do. It would also be nice to have at least one owner sign off.

typescript-bot · 2022-03-21T21:44:15Z

@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-04-04T10:56:57Z

Maybe we could type createServer something like this?

function createServer(options?: Options): Server<IncomingMessage, ServerResponse<IncomingMessage>>
function createServer<T extends IncomingMessage>(options: Options & { IncomingMessage: typeof T }): Server<T, ServerResponse<T>>
function createServer<T extends ServerResponse<IncomingMessage>>(options: Options & { ServerResponse: typeof T }): Server<IncomingMessage, T>
function createServer<T extends IncomingMessage, U extends ServerResponse<T>>(options: Options & { IncomingMessage: typeof T, ServerResponse: typeof U }): Server<T, U>

That would save you having to manually type out the generic parameters when passing custom classes, and it would also ensure that you are actually passing the classes if you define the generic parameters.

Indeed, we can do something like this

interface ServerOptions<
  Request extends typeof IncomingMessage = typeof IncomingMessage,
  Response extends typeof ServerResponse= typeof ServerResponse
> {
  IncomingMessage?: Request | undefined;
  ServerResponse?: Response | undefined;
  maxHeaderSize?: number | undefined;
  insecureHTTPParser?: boolean | undefined;
}
type RequestListener<
  Request extends typeof IncomingMessage = typeof IncomingMessage,
  Response extends typeof ServerResponse = typeof ServerResponse
> = (req: InstanceType<Request>, res: InstanceType<Response>) => void;
function createServer<
  Request extends typeof IncomingMessage = typeof IncomingMessage,
  Response extends typeof ServerResponse = typeof ServerResponse,
>(requestListener?: RequestListener<Request, Response>): Server<Request, Response>;
function createServer<
  Request extends typeof IncomingMessage = typeof IncomingMessage,
  Response extends typeof ServerResponse = typeof ServerResponse,
>(options: ServerOptions<Request, Response>, requestListener?: RequestListener<Request, Response>): Server<Request, Response>;

Does anyone agree with the suggested approach?

typescript-bot · 2022-04-04T11:17:34Z

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

vansergen · 2022-04-04T12:03:35Z

Actually, the suggested way works modulo res.req

class MyIncomingMessage extends http.IncomingMessage {
  foo: typeof foo;
}
class MyServerResponse extends http.ServerResponse<typeof MyIncomingMessage> {
  bar: typeof bar;
}
let server = http.createServer(
  { ServerResponse: MyServerResponse, IncomingMessage: MyIncomingMessage },
  reqListener,
);

produces the following error

Type 'typeof MyServerResponse' does not satisfy the constraint 'typeof ServerResponse'.
  Types of construct signatures are incompatible.
    Type 'new (req: MyIncomingMessage) => MyServerResponse' is not assignable to type 'new <Request extends typeof IncomingMessage = typeof IncomingMessage>(req: InstanceType<Request>) => ServerResponse<Request>'.
      Types of parameters 'req' and 'req' are incompatible.
        Type 'InstanceType<Request>' is not assignable to type 'MyIncomingMessage'.
          Type 'unknown' is not assignable to type 'MyIncomingMessage'.
            Property 'foo' is missing in type 'IncomingMessage' but required in type 'MyIncomingMessage'.

typescript-bot · 2022-04-27T13:25:48Z

@vansergen I haven't seen any activity on this PR in more than three weeks, and it still has problems that prevent it from being merged. The PR will be closed on May 4th (in a week) if the issues aren't addressed.

typescript-bot · 2022-05-05T13:24:34Z

@vansergen To keep things tidy, we have to close PRs that aren't mergeable and don't have activity in the last month. No worries, though — please open a new PR if you'd like to continue with this change. Thank you!

vansergen added 2 commits February 20, 2022 20:40

feat(http): add custom IncomingMessage and ServerResponse support

38c1f13

test(http): add custom IncomingMessage and ServerResponse

1749b3f

vansergen requested a review from peterblazejewicz as a code owner February 20, 2022 18:00

typescript-bot added the Critical package label Feb 20, 2022

vansergen added 4 commits February 20, 2022 21:18

style: add empty spaces

7662362

test(node): add res.req

6164645

test(node): add res.req

3ac12bb

test: update request/response types

3777929

typescript-bot added the Edits multiple packages label Feb 20, 2022

test(node): add createServer

5c11a8e

typescript-bot added the Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. label Mar 4, 2022

amcasey suggested changes Mar 21, 2022

View reviewed changes

typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. labels Mar 21, 2022

Merge pull request #4 from b2broker/master

52c710f

typescript-bot added Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. and removed Revision needed This PR needs code changes before it can be merged. labels Apr 4, 2022

typescript-bot added The CI failed When GH Actions fails and removed Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. labels Apr 4, 2022

typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Apr 27, 2022

typescript-bot closed this May 5, 2022

vansergen mentioned this pull request Aug 24, 2022

[Node]: Update http request listeners #61922

Merged

6 tasks

Conversation

vansergen commented Feb 20, 2022

Motivation

Checklist

Uh oh!

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

Uh oh!

5 packages in this PR

Code Reviews

Status

Inactive

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

SimonSchick commented Feb 26, 2022

Uh oh!

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

Uh oh!

Uh oh!

LinusU commented Mar 1, 2022

Uh oh!

typescript-bot commented Mar 4, 2022

Uh oh!

RyanCavanaugh commented Mar 10, 2022

Uh oh!

typescript-bot commented Mar 11, 2022

Uh oh!

amcasey left a comment

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Mar 21, 2022

Uh oh!

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

Uh oh!

Uh oh!

typescript-bot commented Apr 4, 2022

Uh oh!

vansergen commented Apr 4, 2022

Uh oh!

typescript-bot commented Apr 27, 2022

Uh oh!

typescript-bot commented May 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

typescript-bot commented Feb 20, 2022 •

edited

Loading

typescript-bot commented Feb 20, 2022 •

edited

Loading

vansergen commented Feb 26, 2022 •

edited

Loading

vansergen commented Feb 28, 2022 •

edited

Loading

vansergen commented Apr 4, 2022 •

edited

Loading