Fix missing port in proxy CONNECT when using the default HTTPS port

[segevfiner]

When the target gRPC URI doesn't contain a port, and as such defaults to the default HTTPS port, the code didn't send the port as part of the HTTP CONNECT request when a proxy is configured, leading to proxies rejected the request with a 400 Bad Request, as the request line is malformed.

We saw this while using firebase/firestore which uses gRPC underneath by default, and connect to "firestore.googleapis.com" without a port by default.

Contributed by courtesy of swimm.io

[Omerr]

Indeed it works and solved us a problem

[murgatroid99]

This seems like a reasonable change, functionally, but it could be cleaner:

1. I don't like "443" as a magic string. It would be better to export DEFAULT_PORT from resolver-dns.ts, and reference that here.
2. With nullish coalescing, (targetHostPost.port != null ? targetHostPost.port : '443') can be shortened to targetHostPort.port ?? 443 (or, integrating the previous point, targetHostPort.port ?? DEFAULT_PORT).
3. Use string templates to make the string construction more concise: `${targetHostPort.host}:${targetHostPort.port ?? DEFAULT_PORT}`
4. You are constructing the same string twice, once for options.path and once for headers.Host. Instead create it once, assign it to a variable, and use that variable in the other two lines.

[segevfiner]

This seems like a reasonable change, functionally, but it could be cleaner:

1. I don't like "443" as a magic string. It would be better to export DEFAULT_PORT from resolver-dns.ts, and reference that here.
2. With nullish coalescing, (targetHostPost.port != null ? targetHostPost.port : '443') can be shortened to targetHostPort.port ?? 443 (or, integrating the previous point, targetHostPort.port ?? DEFAULT_PORT).
3. Use string templates to make the string construction more concise: `${targetHostPort.host}:${targetHostPort.port ?? DEFAULT_PORT}`
4. You are constructing the same string twice, once for options.path and once for headers.Host. Instead create it once, assign it to a variable, and use that variable in the other two lines.

Yeah. Wasn't really sure what ECMAScript version this was targeting, but will do.

[murgatroid99]

One of the great things about TypeScript is that it will allow the use of ECMAScript features that are incompatible with the target, and it will transpile shims as necessary. The library currently uses typescript ^5.1.3 per the package.json, so any code that works in that version of TypeScript should be acceptable.

[segevfiner]

One of the great things about TypeScript is that it will allow the use of ECMAScript features that are incompatible with the target, and it will transpile shims as necessary. The library currently uses typescript ^5.1.3 per the package.json, so any code that works in that version of TypeScript should be acceptable.

Yeah. Guess it was late when I debugged this, and didn't even notice I was in TypeScript 😋

[murgatroid99]

OK, that looks good now. If you rebase the change onto the @grpc/grpc-js@1.9.x branch and change the PR target, it will go out in the next patch release.

[segevfiner]

OK, that looks good now. If you rebase the change onto the @grpc/grpc-js@1.9.x branch and change the PR target, it will go out in the next patch release.

Rebased.

[murgatroid99]

This is now out in 1.9.10.