added types to OutgoingHttpHeaders #66783

skwee357 · 2023-09-22T00:50:51Z

Please fill in this template.

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

Select one of these and delete the others:

If adding a new definition:

The package does not already provide its own types, or cannot have its .d.ts files generated via --declaration
If this is for an npm package, match the name. If not, do not conflict with the name of an npm package.
Create it with dts-gen --dt, not by basing it on an existing project.
Represents shape of module/library correctly
tslint.json should contain { "extends": "@definitelytyped/dtslint/dt.json" }, and no additional rules.
tsconfig.json should have noImplicitAny, noImplicitThis, strictNullChecks, and strictFunctionTypes set to true.

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: <>
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.

If removing a declaration:

If a package was never on Definitely Typed, you don't need to do anything. (If you wrote a package and provided types, you don't need to register it with us.)
Delete the package's directory.
Add it to notNeededPackages.json.

skwee357 · 2023-09-22T00:51:30Z

As per the discussion in #66753

typescript-bot · 2023-09-22T00:51:33Z

@skwee357 Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

node — on npm, on unpkg
- owner-approval: @Semigradsky

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

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

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
✅ Most recent commit is approved by type definition owners or DT maintainers

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": 66783,
  "author": "skwee357",
  "headCommitOid": "adfb3b7b9611cf6a68003e22c675d75dd9096e29",
  "mergeBaseOid": "46cd17037ac5399364b0a59511d646c0dd56165a",
  "lastPushDate": "2023-09-22T00:50:51.000Z",
  "lastActivityDate": "2023-09-29T15:20:01.000Z",
  "maintainerBlessed": "Waiting for Code Reviews",
  "mergeOfferDate": "2023-09-29T03:06:27.000Z",
  "mergeRequestDate": "2023-09-29T15:20:01.000Z",
  "mergeRequestUser": "skwee357",
  "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/test/http.ts",
          "kind": "test"
        },
        {
          "path": "types/node/ts4.8/http.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/ts4.8/test/http.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v16/http.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/test/http.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v16/ts4.8/http.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v16/ts4.8/test/http.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v18/http.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v18/test/http.ts",
          "kind": "test"
        },
        {
          "path": "types/node/v18/ts4.8/http.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/node/v18/ts4.8/test/http.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",
        "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": "Semigradsky",
      "date": "2023-09-28T15:53:00.000Z",
      "isMaintainer": false
    },
    {
      "type": "stale",
      "reviewer": "Uzlopak",
      "date": "2023-09-26T17:45:01.000Z",
      "abbrOid": "8bef76d"
    }
  ],
  "mainBotCommentID": 1730564460,
  "ciResult": "pass"
}

typescript-bot · 2023-09-22T00:51:35Z

🔔 @microsoft @DefinitelyTyped @jkomyno @alvis @r3nya @btoueg @smac89 @Touffy @DeividasBakanas @eyqs @Hannes-Magnusson-CK @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @n-e @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 Approve or Request Changes in the GitHub UI so I know what's going on.

Semigradsky · 2023-09-22T04:45:47Z

types/node/http.d.ts

+        'content-disposition'?: string | undefined;
+        'content-encoding'?: string | undefined;
+        'content-language'?: string | undefined;
+        'content-length'?: string | undefined;


number often used for 'content-length'

thanks, fixed

Semigradsky · 2023-09-22T04:47:03Z

types/node/http.d.ts

-    interface OutgoingHttpHeaders extends NodeJS.Dict<OutgoingHttpHeader> {}
+    interface OutgoingHttpHeaders extends NodeJS.Dict<OutgoingHttpHeader> {
+        accept?: string | undefined;
+        'accept-charset'?: string | undefined;


string[] can be used almost with all headers

Is the same applied to IncomingHeaders? If so, I propose we match them, because as of right now, most incoming headers accept only string.

See https://github.com/nodejs/node/blob/31e727db7e0f57bd71d35ce8c1cf378bab309b0f/test/parallel/test-http-server-multiheaders2.js#L32-L45 for multiple allowed headers list

Is the same applied to IncomingHeaders?

Not sure, they can be received as joined string. Need testing

Looks like only string for IncomingHeaders: https://github.com/nodejs/node/blob/31e727db7e0f57bd71d35ce8c1cf378bab309b0f/test/parallel/test-http-multiple-headers.js#L23-L29

typescript-bot · 2023-09-23T16:54:56Z

@Semigradsky 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 · 2023-09-24T01:04:11Z

@skwee357 Unfortunately, this pull request currently has a merge conflict 😥. Please update your PR branch to be up-to-date with respect to master. Have a nice day!

Uzlopak · 2023-09-26T13:38:53Z

types/node/http.d.ts

+        'content-disposition'?: string | undefined;
+        'content-encoding'?: string | undefined;
+        'content-language'?: string | undefined;
+        'content-length'?: string | number | undefined;


This assumption is dangerously wrong. headers are usually strings and have to be casted manually.

In examples and tests Node.js team often use number for 'content-length'. The same in real world. Not sure that need to break it.

Do you have a link?

In examples:

https://nodejs.org/dist/latest-v20.x/docs/api/http.html#responsewriteheadstatuscode-statusmessage-headers

https://nodejs.org/dist/latest-v20.x/docs/api/http2.html#http2streamrespondwithfdfd-headers-options

In tests:

https://github.com/nodejs/node/blob/c829c03df2451212d1d6d28e867579e5d5e4a06e/test/async-hooks/test-http-agent-handle-reuse-serial.js#L63

https://github.com/nodejs/node/blob/c829c03df2451212d1d6d28e867579e5d5e4a06e/test/parallel/test-http-allow-content-length-304.js#L11

https://github.com/nodejs/node/blob/c829c03df2451212d1d6d28e867579e5d5e4a06e/test/parallel/test-http-client-spurious-aborted.js#L14

and many others...

Ok, it will converted to string internally...

typescript-bot · 2023-09-26T16:42:36Z

@Uzlopak, @Semigradsky 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 · 2023-09-26T17:20:05Z

@Semigradsky, @Uzlopak 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 · 2023-09-26T17:44:32Z

@Uzlopak, @Semigradsky 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?

Uzlopak · 2023-09-26T17:45:01Z

types/node/http.d.ts

+        'sec-websocket-protocol'?: string | string[] | undefined;
+        'sec-websocket-version'?: string | undefined;
+        server?: string | undefined;
+        'set-cookie'?: string[] | undefined;


Imho set-cookie can be also a single string.

Apologies for the back-and-forth. I'm not focused recently

Semigradsky · 2023-09-27T05:45:46Z

@skwee357 Looks good! 👍 Currently you added it for Node.js v20 and TypeScript > 4.8

Need to add also

to folder types/node/ts4.8 for TypeScript <= 4.8
to folders types/node/v18, types/node/v18/ts4.8, types/node/v16, types/node/v16/ts4.8 for other Node.js versions

typescript-bot · 2023-09-29T03:06:28Z

@Uzlopak 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?

skwee357 · 2023-09-29T15:20:01Z

Ready to merge

JandenMa · 2023-09-29T18:52:58Z

It is not compatible with the header type of aws4. It broke our deployment. 😔 @mhart

skwee357 · 2023-09-30T00:03:58Z

@JandenMa I apologize for that.
My understanding is that aws4 expects accept header to be a string, rather than string[]?

As pointed out by @Semigradsky here https://github.com/nodejs/node/blob/31e727db7e0f57bd71d35ce8c1cf378bab309b0f/test/parallel/test-http-server-multiheaders2.js#L32-L45 accept header could be an array.

As a temporary solution, maybe you can force cast it to satisfy the problematic package? I would also suggest opening an issue / PR to the problematic package.

JandenMa · 2023-09-30T04:47:31Z

@skwee357 Thanks for your response.
I think aws4 depends on the old @types/node version.
And no worries I use resolutions to force indirect dependencies to install 18.18.0, it works for now.
Could you let me know when the problematic package is upgraded or resolved?

Semigradsky · 2023-09-30T08:43:50Z

@JandenMa what the type HeadersInit?

JandenMa · 2023-09-30T08:53:24Z

@Semigradsky

Semigradsky · 2023-09-30T09:04:16Z

Ok, looks like aws4 is okay and just HeadersInit from lib.dom.d.ts and ...HttpHeaders from @types/node incompatible. How you use these headers then? Maybe you not need type HeadersInit here.

JandenMa · 2023-09-30T09:10:41Z

Well I use the createHttpLink from the @apollo/client for signable requests, the options of fetch argument requires RequestInit type, and the headers of options requires HeadersInit type. @Semigradsky

Semigradsky · 2023-09-30T09:41:20Z

Do you use node-fetch? Or 'native' fetch from Node.js?

JandenMa · 2023-09-30T09:41:43Z

native one~

Semigradsky · 2023-09-30T10:01:20Z

In this case looks like issue should be gone after #66824 when you will use types from @types/node instead of fetch from lib.dom.d.ts.

JandenMa · 2023-09-30T10:10:14Z

Hmmm I don't think that's what I can control though.
The issue I think from aws4.sign, the headers from it that seems different type from the @types/node change. But I could be wrong.

Semigradsky · 2023-09-30T10:12:09Z

No, they are the same, see

DefinitelyTyped/types/aws4/index.d.ts

Line 38 in 5d599be

headers?: OutgoingHttpHeaders;

JandenMa · 2023-09-30T10:20:42Z

Oh, it refers to the same type.
Alright, I will try to import types from @types/node instead of lib.dom.d.ts (global) when #66824 is merged. That's what you meant right?
Btw, could you let me know after merging?

JandenMa · 2023-09-30T10:31:42Z

Thank you @Semigradsky 😋

due to DefinitelyTyped/DefinitelyTyped#66783 we got a downstream type error on httpExecutor's RequestHeaders fixed by using OutgoingHttpHeader from node bumped repo's @types/node to verify fix

added types to OutgoingHttpHeader

7178bb5

skwee357 requested review from eps1lon and peterblazejewicz as code owners September 22, 2023 00:50

typescript-bot added the Critical package label Sep 22, 2023

typescript-bot added this to Waiting for Code Reviews in New Pull Request Status Board Sep 22, 2023

typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Review in New Pull Request Status Board Sep 22, 2023

Semigradsky reviewed Sep 22, 2023

View reviewed changes

sheetalkamat moved this from Needs Maintainer Review to Waiting for Code Reviews in New Pull Request Status Board Sep 22, 2023

added content length number header

a9e88b6

typescript-bot added the Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. label Sep 24, 2023

typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Sep 24, 2023

typescript-bot removed the Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. label Sep 24, 2023

typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Sep 24, 2023

Uzlopak reviewed Sep 26, 2023

View reviewed changes

fix conflicts

905f85b

skwee357 force-pushed the typed-headers branch from 623f763 to 905f85b Compare September 26, 2023 16:03

added array headers option

8bef76d

Uzlopak reviewed Sep 26, 2023

View reviewed changes

set-cookie single string

234e836

skwee357 mentioned this pull request Sep 26, 2023

types: added http header types to reply fastify/fastify#5046

Merged

4 tasks

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

Merge branch 'DefinitelyTyped:master' into typed-headers

adfb3b7

typescript-bot removed the The CI failed When GH Actions fails label Sep 29, 2023

typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Sep 29, 2023

typescript-bot added the Self Merge This PR can now be self-merged by the PR author or an owner label Sep 29, 2023

typescript-bot moved this from Waiting for Code Reviews to Waiting for Author to Merge in New Pull Request Status Board Sep 29, 2023

typescript-bot moved this from Waiting for Author to Merge to Recently Merged in New Pull Request Status Board Sep 29, 2023

typescript-bot merged commit 9189e31 into DefinitelyTyped:master Sep 29, 2023
2 checks passed

skwee357 deleted the typed-headers branch September 29, 2023 15:30

typescript-bot removed this from Recently Merged in New Pull Request Status Board Sep 29, 2023

Semigradsky mentioned this pull request Sep 30, 2023

[node] Add fetch types #66824

Merged

8 tasks

ekwoka mentioned this pull request Oct 1, 2023

📝 Improves Documentation of Alpine Methods #66890

Closed

8 tasks

AviVahl mentioned this pull request Oct 1, 2023

fix: compat with newest @types/node electron-userland/electron-builder#7806

Merged

added types to OutgoingHttpHeaders #66783

added types to OutgoingHttpHeaders #66783

Conversation

skwee357 commented Sep 22, 2023 • edited

skwee357 commented Sep 22, 2023 • edited

typescript-bot commented Sep 22, 2023 • edited

1 package in this PR

Code Reviews

Status

typescript-bot commented Sep 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Semigradsky Sep 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typescript-bot commented Sep 23, 2023

typescript-bot commented Sep 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typescript-bot commented Sep 26, 2023

typescript-bot commented Sep 26, 2023

typescript-bot commented Sep 26, 2023

Uzlopak Sep 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Semigradsky commented Sep 27, 2023

typescript-bot commented Sep 29, 2023

skwee357 commented Sep 29, 2023

JandenMa commented Sep 29, 2023 • edited

skwee357 commented Sep 30, 2023

JandenMa commented Sep 30, 2023

Semigradsky commented Sep 30, 2023

JandenMa commented Sep 30, 2023

Semigradsky commented Sep 30, 2023

JandenMa commented Sep 30, 2023

Semigradsky commented Sep 30, 2023

JandenMa commented Sep 30, 2023

Semigradsky commented Sep 30, 2023

JandenMa commented Sep 30, 2023

Semigradsky commented Sep 30, 2023 • edited

JandenMa commented Sep 30, 2023

JandenMa commented Sep 30, 2023

skwee357 commented Sep 22, 2023 •

edited

skwee357 commented Sep 22, 2023 •

edited

typescript-bot commented Sep 22, 2023 •

edited

Semigradsky Sep 26, 2023 •

edited

Uzlopak Sep 26, 2023 •

edited

JandenMa commented Sep 29, 2023 •

edited

Semigradsky commented Sep 30, 2023 •

edited