[node] Update older node types (7,6,4) for more consistency with 8

[kevin-greene-ck]

• Specifically add interfaces for reqeust headers
• Make the naming of client request options consistent (ClientRequestArgs)

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.)
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

• Provide a URL to documentation or source code which provides context for the suggested changes: [thrift] Remove dependency on node v8 #20579
• Increase the version number in the header if appropriate.
• If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

[dt-bot]

types/node/v4/index.d.ts

to authors (@DefinitelyTyped/DefinitelyTyped Microsoft TypeScript (account can't be detected)). Could you review this PR?
👍 or 👎?

---

types/node/v6/index.d.ts

to authors (@DefinitelyTyped/DefinitelyTyped @WilcoBakker @inlined Microsoft TypeScript (account can't be detected)). Could you review this PR?
👍 or 👎?

---

types/node/v7/index.d.ts

to authors (@DefinitelyTyped/DefinitelyTyped @parambirs @robdesideri @tellnes @WilcoBakker Microsoft TypeScript (account can't be detected)). Could you review this PR?
👍 or 👎?

[typescript-bot]

Approved by a listed owner. PR ready to merge pending express review by a maintainer.

[inlined]

Can try to review this tomorrow.

[inlined]

I'll need some time to do a deeper review on the actual conformance to the docs of each version, but there is a high-level issue:

This PR renames a type in the http package (though oddly it does not rename the corresponding type in the https package). Since this code is in active use for developers it will be considered a breaking change.

I personally don't mind adding definitions to what previously was an any but renaming RequestOptions to ClientRequestArgs seems to go too far. We should either drop that part of the change or add a type alias for existing code (I would love other reviewers to chime in)

[typescript-bot]

@kevin-greene-ck Please fix the failures indicated in the Travis CI log.

[typescript-bot]

@inlined - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

[inlined]

Thanks for the patience and the contribution. Just a few nits. I'd prefer to use a slightly more obscure type that's more technically correct since Node only returns an array of string for a single header name.

[inlined]

I could not find a reference to undefined. What is the behavior here?

[kevin-greene-ck]

Yeah, this probably shouldn't be included. It doesn't blow things up. But it does put the string literal 'undefined' as the header value. I removed it.

[Flarna]

The reason for adding undefined here is to allow users to augment the interface like this to get code completion for often used headers:

interface OutgoingHttpHeaders  {
  age?: string;
  host?: string;
}

if | undefined is not added in the index signature it's only possible to add mandatory properties which is useless.

[inlined]

Thanks for the clarification.

[inlined]

https://nodejs.org/docs/latest-v4.x/api/http.html#http_message_headers says that the only header that will be an array of string is set-cookie and that it will always be an array of string. This admittedly took me a long time to figure out but:

export type HTTPHeader = {
  [header: string]: string
} & {
  'set-cookie'?: string[]
}

enforces that constraint

[kevin-greene-ck]

Good catch.

[inlined]

Same comment as v4

[inlined]

And same two comments here again too.

[Flarna]

Could you please split this into two interfaces and and combine them to a type then to allow augmentation. Additionally it would be nice if this could be also added to Node 8 (as the main target here is anyway to sync them),

[inlined]

I was on the fence about this honestly. The type names are pretty useless (e.g. we wouldn't expect a docs page on them by themselves) and I've seen IDEs do a worse job at code complete or mouseover docs once the subtypes in a composite type are named.

[kevin-greene-ck]

This actually doesn't work. I was adding tests for this and I get the same "Property ''set-cookie'' is incompatible with index signature." I would expect if I added the set cookie property onto IncomingHttpHeaders like this

type IncomingHttpHeaders {
    [header: string]: string;
    'set-cookie'?: string[];
}

For this to work the index signature must account for string | string[]. Is there any reason to still use an Intersection type?

[inlined]

Don't ask me why, but

type IncomingHttpHeaders {
    [header: string]: string;
    'set-cookie'?: string[];
}

is a compiler error while

type IncomingHttpHeaders {
    [header: string]: string;
} & {
    'set-cookie'?: string[];
}

is not. Types do not allow properties to have a different value type as the index value type, but you can get around this with composite types. Verified the type definition and variable instantiation compile cleanly on TS 5.3.0.

[kevin-greene-ck]

The type definition compiles in the second example, but usage of the type does not. That's why I didn't catch it until I was adding tests for it.

At least this test doesn't compile with typescrpt@2.5.3

const headers: http.IncomingHttpHeaders = {
            'Content-Type': 'application/json',
            'set-cookie': [ 'type=ninja', 'language=javascript' ]
};

[Flarna]

What a pity. As we have to include the array in the index signature anyway I see no reason for having two types. To get the best out of it we could add other header names besides set-cookie to force them to string. Users can add more by augmenting the interface for their private header names. e.a.:

interface IncomingHttpHeaders extends Partial<Record<"age" | "host" | "date", string>> {
    'set-cookie'?: string[];
    [name: string]: string | string[] | undefined;
}

Or using copy/paste instead Partial<Record<>> which is maybe easier to read:

interface IncomingHttpHeaders {
    age?: string;
    host?: string;
    //...
    'set-cookie'?: string[];
    [name: string]: string | string[] | undefined;
}

Again the | undefined is needed here to allow adding optional properties.

[kevin-greene-ck]

Yeah, the list of common headers isn't too long I can add those to string and users can extend.

[Flarna]

All outgoing headers can be specified as array by user, not just set-cookie. This differs from incoming headers where node combines them during parsing.

[inlined]

My comments are incorporated and I'm going to be AFK for a while. Passing the baton to @Flarna who had some good comments. Please come to an agreement on whether the composite type should be built on anonymous or named types before checking in.

[kevin-greene-ck]

@sheetalkamat After resolving merge conflict travis is trying to run tests on all packages instead of just my changes. Is there something I can do about this? Testing the node package works locally.

[Flarna]

@kevin-greene-ck I'm sorry but I think the root cause here is my request to sync back to node 8. >1000 modules depend on latest node and therefore all of them are tested and this often results in unrelated fails.

[kevin-greene-ck]

Ah yes, I guess that merge commit was the first time travis ran after I made changes to node 8. Hmm. Thanks for the insight.

[RyanCavanaugh]

@kevin-greene-ck Please fix the failures indicated in the Travis CI log.

[kevin-greene-ck]

@RyanCavanaugh Everything is fixed, but travis times out.