lib: destructuring assignment instead of require whole module

[ZYSzys]

Minor refactor: Use destructuring assignment instead of require the whole module.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[TimothyGu]

LGTM.

Do we want to semver-major this? This no longer allows monkey-patched util._extend to take effect when createSocket is called, for example.

[jasnell]

Yes, if this impacts the ability to monkey-patch, then it really should be semver-major and we should make sure we run CITGM.

[joaocgreis]

Last CI run was not successful for unrelated reasons (nodejs/build#1495).
Resumed: https://ci.nodejs.org/job/node-test-pull-request/17197/

[addaleax]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1547/

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/18666/

[Trott]

CITGM results are no longer available so I guess this needs another CITGM run...

[Trott]

CITGM: https://ci.nodejs.org/job/citgm-smoker/1631/

[ZYSzys]

@Trott Ah...Is there something wrong ? I'm willing to fix it.

[Trott]

@Trott Ah...Is there something wrong ? I'm willing to fix it.

Not necessarily. CI is green. CITGM is red, but it always is and requires a bit of care and/or domain knowledge to interpret. It might be totally fine. @nodejs/citgm

[Trott]

If CITGM is good, then this is ready to land. @nodejs/citgm https://ci.nodejs.org/job/citgm-smoker/1631/

[antsmartian]

It needs a rebase @ZYSzys

[ZYSzys]

@antsmartian Done:)

[Trott]

Post-rebase CI: https://ci.nodejs.org/job/node-test-pull-request/19758/ ✔️

[BridgeAR]

@nodejs/tsc PTAL

[mcollina]

LGTM

[Trott]

Is removing the ability to monkey-patch a desirable feature or an incidental side-effect?

[mcollina]

I’d like a review from @nodejs/diagnostics. Is this used by APMs to instrument http?

[Qard]

async-listener, and therefore most APM vendors which depend on it through continuation-local-storage, patch elsewhere in net and http, so I don't think this would be a problem.

[Flarna]

We don't have the need to patch net.createConnection currently so it should be no issue now.

But please be aware that monkey patching is the only entry point for APMs this time as javascript has nothing like JVMTI. Therefore changes like this have the ability to break several APMs - with the risk that there are no workarounds available (marking it as semver-major doesn't add workarounds).

Besides that I find net.createConnection is better readable then just createConnection as it's clear that something from net is used. I would us the shortcuts via destruction only for utility function used frequently in a file. But this is just my personal opinion here.

[Trott]

Besides that I find net.createConnection is better readable then just createConnection as it's clear that something from net is used.

TBH I find most (but not all) changes to destructuring problematic for exactly that reason. Not enough for me to block them, though.

We don't know that this will break anyone's code but we can't be sure. Unless this is a definite improvement, I would be inclined to not do it for that reason. It's why I asked "Is removing the ability to monkey-patch a desirable feature or an incidental side-effect?" If a feature, great. If a side-effect, let's not do this. Aesthetic/cosmetic changes that might break someone's code seems like a bad trade-off.

/ping @addaleax as I believe she has opinions on this type of thing. (And apologies if I'm wrong about that.)

[addaleax]

@Trott I don’t think I have much to add to what you were saying here. 👍

[mcollina]

It’s probably a good idea to not do this change at all.

[ZYSzys]

Hey, everyone, thanks all of you for your reviews firstly.

Learn a lot from the discussion, so if It’s not a suitable change, feel free to close this PR.

Thank you sincerely again.👍