fix: connector types

[S222em]

This PR fixes an issue with the type buildConnector.BuildOptions.

The buildConnector.BuildOptions are used for 2 different methods, tls.connect(opts) for https, and net.connect(opt) for all other protocols. TlsOptions does not include all possible options that can be passed to those methods.

• tls.connect(opt) accepts the type ConnectOptions
• net.connect(opt) accepts the type NetConnectOpts

[mcollina]

We test our types! Can you add an assertion for this using tsd?

[mcollina]

Also the build is failing

[S222em]

Okay, so @KhafraDev the interface can't actually extend both of the types, it must be type some = 1 | 2. Will it really be a problem? Explained below why it cant be like that:

The type of NetConnectOpts is actually not a interface but a type some = 1 | 2, which could be made into this:

export interface BuildOptions extends ConnectionOptions, TcpNetConnectOpts, IpcNetConnectOpts {
    maxCachedSessions?: number | null;
    socketPath?: string | null;
    timeout?: number | null;
  }

But this gives a TS error that properties of Tcp and Ipc opts can't be both used due to conflicting types.

Then we would get back to this:

export type BuildOptions = (ConnectionOptions | NetConnectOpts) & {
    maxCachedSessions?: number | null;
    socketPath?: string | null;
    timeout?: number | null;
  }

Which could also be:

export type BuildOptions = (ConnectionOptions | TcpNetConnectOpts | IpcNetConnectOpts ) & {
    maxCachedSessions?: number | null;
    socketPath?: string | null;
    timeout?: number | null;
  }

[S222em]

@mcollina what's your opinion on this? ^

[mcollina]

I think the following look correct:

export type BuildOptions = (ConnectionOptions | TcpNetConnectOpts | IpcNetConnectOpts ) & {
    maxCachedSessions?: number | null;
    socketPath?: string | null;
    timeout?: number | null;
  }

however I'm not a big expert on TS, so I defer to whatever pleases the tests.

[codecov-commenter]

Codecov Report

Merging #1362 (b3e23d4) into main (f70ac7f) will increase coverage by 0.45%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1362      +/-   ##
==========================================
+ Coverage   94.24%   94.70%   +0.45%     
==========================================
  Files          45       49       +4     
  Lines        4204     4246      +42     
==========================================
+ Hits         3962     4021      +59     
+ Misses        242      225      -17     

Impacted Files	Coverage Δ
lib/fetch/util.js	79.51% <0.00%> (-0.49%)	⬇️
lib/client.js	97.89% <0.00%> (-0.04%)	⬇️
index.js	100.00% <0.00%> (ø)
lib/core/errors.js	100.00% <0.00%> (ø)
lib/proxy-agent.js	100.00% <0.00%> (ø)
lib/api/api-request.js	100.00% <0.00%> (ø)
lib/fetch/constants.js	100.00% <0.00%> (ø)
lib/mock/mock-agent.js	100.00% <0.00%> (ø)
lib/mock/mock-utils.js	100.00% <0.00%> (ø)
lib/mock/mock-interceptor.js	100.00% <0.00%> (ø)
... and 12 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f70ac7f...b3e23d4. Read the comment docs.

[mcollina]

Can you add a couple of assertions for this in the types tests?

[mcollina]

lgtm

[kibertoad]

@ronag Should this be merged already?

[mcollina]

@kibertoad is this ok? I'm waiting for somebody to better knowledge of TS than myself.

[kibertoad]

@mcollina This is good to go!