socketPath option is not passed to the connect function

[zdm]

Bug Description

In Pool dispatcher socketPath option is not passed to the connect if connect is a function.

Reproducible By

const dispatcher = new Agent ( {
    socketPath: "/var/run/socket",
    connect ( options, callback ) {
        console.log( options );
        process.exit();
    }
} )

await fetch( "http://local/", {
    dispatcher
} );

Expected Behavior

Expecting to see socketPath in options, passed to the connect function.

When connect is specified as object - it works, but for function you just forget to pass socketPath.

Logs & Screenshots

Environment

Additional context

[metcoder95]

Good catch, would you like to send a PR?

[Uzlopak]

@metcoder95
Actually i doubt that this is a bug. If I read the code it looks like it was on purpose not allowed to manipulate the socketPath.

[zdm]

Hmm, bit if we specify socketPath, connect function should receive if, or how it should work?

[metcoder95]

Well, in theory it should, we are just not passing it to the connect factory .

We have documented that we support these options to the default connector factory. I'd also expect that they are passed down to the custom factory that its submitted.

[zdm]

Ok. what is your decision?
I cam make PR it this is a bug.

[zdm]

I think it should be fixed.
If user do not want to change socketPath in the future - he can use closure, but by default this parameter should pass.

[mcollina]

I think a PR with the change would help us spot why we did not pass it. If it's that's the case there is likely a test to cover it.

[metcoder95]

+1

[Uzlopak]

What happens if you specify some file like /etc/passwd as socketPath?

[zdm]

What happens if you specify some file like /etc/passwd as socketPath?

Error, because this is not a socket.

[zdm]

So, I carefully looked at the code.
sockerPath should behave like localAddress option.
It should be removed from buildConnector options and added to the connect call options.
This will be a breaking change because type script interfaces will be changed.

[zdm]

timeout also not passed to connect function.
I think all options, which are passed to the buildConnector should be passed to the connect function without exclusions.

[metcoder95]

We are preparing the cut for new major, so it should be ok to do breaking changes.

Feel free to move forward with a PR so we can iterate over it

[Uzlopak]

It is actually breaking if we add more options?

[metcoder95]

I think refers to removing it from buildConnector.

Not so sure we should do that, but maybe I'm overseeing something; happy to see the PR to validate