do not pollute the node typing

[dcharbonnier]

This prevent any other module to implement IncomingMessage with a timing property (example request)

declare module 'http' {
    interface ClientRequest {
        timings?: Timings;
    }

    interface IncomingMessage {
        timings?: Timings;
    }
}

[szmarczak]

What's the actual problem here?

[szmarczak]

Could you please answer the question above? If that's a bug, please include reproducible code.

[dcharbonnier]

you're polluting the typing of nodejs this is not possible, no need to reproduce anything this is not a possible behavior. it's like redefining javascript functions with your own implementation. the concept is wrong. Then concretely, install let's say @kubernetes/client-node with your project on a typescript projet and you will see the consequencies. Then closing a PR of someone that took the time to help on an opensource project because you don't understand what you're doing before asking for some help to understand is not exactly what I personally expect .

[szmarczak]

The change is too breaking. Got uses request.timings and response.timings already. If the change is necessary, I'd rather go with ClientRequest<TimingsType = never>. I'll try to see if there's a problem. I'll get back to you shortly.

[szmarczak]

what you're doing before asking for some help to understand is not exactly what I personally expect .

As mentioned before, those changes are unnecessary. If this is a problem indeed, then it can be solved in another way.

Then concretely, install let's say @kubernetes/client-node with your project on a typescript projet and you will see the consequencies.

What info do I have here? I can't see the error you're facing. I did install @kubernetes/client-node and the following code compiles successfully:

import got from 'got';
import k8s = require('@kubernetes/client-node');

const kc = new k8s.KubeConfig();
kc.loadFromDefault();

const k8sApi = kc.makeApiClient(k8s.CoreV1Api);

(async () => {
    k8sApi; // ...

    const {body} = await got('https://httpbin.org/anything');

    console.log(body);
})();

If you can set up an example repository of the problem, surely that would be a big step forward. I wish I could help, but without a clear look of the issue I can't. Even a code example would be great.

[dcharbonnier]

git clone https://gist.github.com/0222ae0b2254c240042031dcbd883377.git
cd 0222ae0b2254c240042031dcbd883377
npm i
./node_modules/.bin/tsc index.ts

[dcharbonnier]

your'e missing the typing for @kubernetes/client-node that's why your example "kind of" works

[szmarczak]

Thank you for your effort, I really appreciate it! Now that I see the problem clearly, I propose a less-breaking solution: maybe let's just export interfaces ClientRequestWithTimings and IncomingMessageWithTimings? So people can just do const typedResponse = response as IncomingMessageWithTimings without a problem :)

[szmarczak]

Could you review #14 please?

[dcharbonnier]

This is not the proper way of doing it but I understand the concerns about the compatibility and what you did solve the immediate issue. Symbol is the way to go in this case, but if we only have one lib doing this incorrect thing it will work. You could also implement the version with the symbol and create a descriptor with a deprecate (https://nodejs.org/api/util.html#util_util_deprecate_fn_msg_code) to access the "timing" property and keep the compatibility for a while.

[szmarczak]

I agree that Symbol would be best as it would not break anything, but response.timings is simpler to use IMO (you can see request has got its own IncomingMessage too), that's what people are familiar with. Not to mention that thousands of people (if not millions) have been using it already.