child_process: revert exposing child process constructor

[eljefedelrodeodeljefe]

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• the commit message follows commit guidelines

Affected core subsystem(s)

child_process

Description of change

The constructor blocks a rewrite of child process behind
require('child_process').ChildProcess, which would be a deprecation
or legacy friendly way. Also exposing was not a good decision in
general and rather fell through code review with bigger changes.

TBH code review failed here, because this small change was embedded in a pretty big one. Things like that should be handed in in 2 PRs #1760.

The consequence is that probably just one consumer, needs to add some 3 lines back in again. Also the constructor is not documented and I highly doubt that we should have back channeling of some known users just to make their lives a little easier. The tiny part is misconduct even though one with tiny impact.

cc @evanlucas, @chrisdickinson, @isaacs

[addaleax]

Not against this per se, but this should not happen without a proper deprecation cycle and without replacement functionality that serves the use cases that were addressed by the original change.

[addaleax]

Also, this is probably obvious, but the constructor is exposed anyway. Having it directly exported by the module just eases access to it.

[evanlucas]

Also exposing was not a good decision in general and rather fell through code review with bigger changes.

Care to clarify why you think this?

TBH code review failed here, because this small change was embedded in a pretty big one. Things like that should be handed in in 2 PRs #1760.

How did code review fail? The PR was open for 7 days before being merged. A member of the TC reviewed it and signed off on it.

Also the constructor is not documented and I highly doubt that we should have back channeling of some known users just to make their lives a little easier.

I'm not sure what you are referring to here. A public issue was opened where this was requested.

The tiny part is misconduct even though one with tiny impact.

Care to be more specific?

[eljefedelrodeodeljefe]

Yeah. My point is that this was a really bad change since there are multiple ways of getting the constructor, but there is no obvious and I think also no user for this. In fact one could use module.exports = child_process.ChildProcess; (<- or anything else on the right hand sight) and module.exports.ChildProcess for more useful things, especially new behaviours. This was against "best code as possible" and is extremely limiting.

[eljefedelrodeodeljefe]

It was buried in a +935 −848 diff and got commented on only once, with no reasoning whether this should be exported or not or in what what way. The discussion in general was really short, given what usual traffic is on the PRs usually.

Also TC members do fail sometimes and an LGTM should sometimes be "have review thoroughly it doesn't break anything and is also good for future purposes and has meaningful value".

It was opened by one person who is well known in the scene, apart from that person I don't see anyone really making any use of it. Only the persons attending the PR could have known of that kind feature, since there was no further documentation.

Well the last comment means, that this was reaaaaaally bad what everyone did, but fortunately there won't be many consequences. However this can serve as an example how things should not happen.

[eljefedelrodeodeljefe]

Alternatively we could move the exports to module.exports = child_process.ChildProcess, since then the module would expose all the old behaviour through the flat API of require('child_process') and any new behaviour could go into require('child_process').ChildProcess

[Fishrock123]

Sorry, I'm not seeing what is wrong with this? This is intended so that libraries can wrap the constructor easier?

The tiny part is misconduct even though one with tiny impact.

Wait what are you claiming here?

[eljefedelrodeodeljefe]

This is written several times now above. It's @isaacs posting a doubtfully useful request and immediate sign-off. Without considering alternatives, discussing it in a proper way and not even documenting it.

[eljefedelrodeodeljefe]

As written above also this doesn't allow having a proper rewrite and hiding it in require('child_process').ChildProcess for the next couple of years.

[eljefedelrodeodeljefe]

As written above also it could be moved to module.exports = child_process.ChildProcess if really necessary. You can and always could get the constructor anyhow in several ways.

[evanlucas]

It's @isaacs posting a doubtfully useful request and immediate sign-off.

@eljefedelrodeodeljefe ok so just to be clear. Just because you don't find something useful, doesn't mean that it isn't useful.

Also, as I have mentioned already, the sign-off was not immediate. The PR was open for 7 days.
The procedure in place during the time of which this was landed was followed. A PR was opened, no one objected. A TC member signed off.

As written above also this doesn't allow having a proper rewrite and hiding it in require('child_process').ChildProcess for the next couple of years.

Yea, I whole-heartedly disagree with this. Any rewrite will have to be at least somewhat backwards compatible, otherwise you will break a lot of the ecosystem.

I also will say that I don't appreciate what you have insinuated in these posts. A lot of us don't get paid to work on core and this kind of shit makes it a lot harder to take time away from my family to try and make node better.

I don't see how not landing this revert prevents one from rewriting child_process, so -1 from me

[eljefedelrodeodeljefe]

I don't either but not getting paid is not justification to neglect any need of improving APIs and then also do such a thing. Technically your change back then might be okay, and not against the rules, but it also forcefully blocks any chance of improvement. child_process are so goddamn convoluted and impossible to improve on and no-ones taking any action. I don't even care if that lands or not, but I care for the greater thing and I don't see anyone associated with systems stuff.

I hope next time something like this comes up you think about design.

[eljefedelrodeodeljefe]

Closing and hoorays for this and setupChannel() and normalizeExecArgs() . Gonna attempt rewrite under module.exports = newCosntructor

[evanlucas]

I don't either but not getting paid is not justification to neglect any need of improving APIs and then also do such a thing

You are making some very accusatory claims here with not much to back them up.

Technically your change back then might be okay, and not against the rules, but it also forcefully blocks any chance of improvement.

Please stop saying this until you can actually justify your argument. Also, the way that change happened would happen the exact same today. If no one objects and someone steps up and signs off on a change, then we merge it in. If someone objects, then an attempt at consensus should be made. If that cannot happen, then it gets escalated to the CTC.

I hope next time something like this comes up you think about design.

Design was considered when this was landed. It exposed the constructor. The other changes were for internal cleanup.

Perhaps next time, try to be less confrontational and insulting and you may get better results...

[Fishrock123]

Let's make sure this doesn't devolve into insults.

The PR is question went through proper review, and the functionality was previously deemed useful by multiple members of the core team. Calling "misconduct" on something is a big deal, fwiw. I suggest avoiding it.

I'm not seeing any constructive way of going forward with this, personally.

The constructor shouldn't usually be used on it's own but having easy access for wrapping via libraries is very useful.

[eljefedelrodeodeljefe]

@evanlucas @Fishrock123 how satisfied are you with the implementation of child_process? When I would come up with a decent rewrite would you (as seen a lot) defer to userland and keep it as it is?

[evanlucas]

That is not something on which a decision could be made without actually seeing the code. The general rule is to keep core small. That is noted in the onboarding document. A rewrite of child_process would also need to be reviewed by more than just me and @Fishrock123 though, especially if it has breaking changes.

[Fishrock123]

@eljefedelrodeodeljefe Refactors are usually fine, but we can't change the exposed API too much (needs deprecation cycles etc), depending on downstream usage.

[Fishrock123]

Please also keep in mind that lots of core is a compromise in what we are able to do between interested parties.

[eljefedelrodeodeljefe]

Yeeah that's all good and sound. But the question is rather whether this will be an immediate deferring, because "child process is good as it is"(?). Because I very much think it is the diametrical opposite.

No, I would keep the API as it is, but just adding the new API to module.exports = NewAPI and that entry point would be the const ChildProcess = require('child_process') instead of the various require('child_process').(method|constructor).

Yes, but it should be an easy compromise to decide, whether the current implementation is stuck and not good, yet it served its purpose since 2009, but it's time to start and integrate something new with all the learnings, while not breaking a anything and also being small in SLOC.

[eljefedelrodeodeljefe]

Rewrite then would be the wrong word.

[Fishrock123]

¯_(ツ)_/¯ hard to know without seeing what you mean

if you think it requires higher-level discussion maybe file an EP for it with that level of detail?

[eljefedelrodeodeljefe]

sigh okay. Probably that's the best ahead-of-time answer I will get.