Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

export child process constructor #2419

Closed
wants to merge 1 commit into from

5 participants

@polotek

This might not be a good idea, but I'm looking to subclass ChildProcess. I think it should be exported from the module unless there's a real good reason not to do it.

@bnoordhuis

I think it should be exported from the module unless there's a real good reason not to do it.

It's the other way around: it should not be exported unless there's a really good reason to do it - and I don't see it, sorry.

@bnoordhuis bnoordhuis closed this
@polotek

Can you explain why you take this position? There's nothing special about the ChildProcess constructor other than the fact it is backed by a binding. It's no different from exposing Socket from net or Server from http. People have used these to great effect. And I think exposing it is in keeping with node being a low level platform where you get all the tools you need.

@bnoordhuis

I don't want people to depend on what is essentially an implementation detail. Exposing the constructor means supporting it indefinitely. Legacy is the only reason net.Socket and net.Server are exposed, I'd axe them if I could.

@polotek
@isaacs
Owner

I'm with @bnoordhuis on this.

The "good reason not to do it" is that every function and class that we expose is an API that has to be supported. Often it seems trivial to expose something if it could be useful. However, switching something from an "internal implementation detail" to "public documented part of the official API" is in fact a decision that increases the maintenance cost of node.

I think at this point in node's maturity, we need to start reining in the api surface, rather than expanding it. That's not to say that nothing can ever change, but increases in surface area need a very high degree of justification.

@mikeal

I agree and disagree with all of you.

Yes, core should not solidify the API for a constructor until we are confident it won't change again.

No, core should not hesitate from exposing core constructors for consumption.

Solution:

exports._ChildProcess = ChildProcess

It's an internal property that is exposed. Modifying it is likely to break shit, use at your own peril because the API is subject to change.

@yorkie

Hey guys there, sometimes we maybe do useful check for child_process object like this:

function xxx(cp) {
  if (!(cp instanceof ChildProcess))
    throw new Error('ChildProcess required');
  //...
}

Even though we could get this in another way, but it's much better for reader, I also know the API exposure problem is very important, maybe we need do it as @mikeal said?

@piscisaureus piscisaureus referenced this pull request in nodejs/io.js
Closed

Expose the ChildProcess constructor #1751

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 25, 2011
  1. @polotek
This page is out of date. Refresh to see the latest.
Showing with 2 additions and 0 deletions.
  1. +2 −0  lib/child_process.js
View
2  lib/child_process.js
@@ -500,3 +500,5 @@ ChildProcess.prototype.kill = function(sig) {
// TODO: raise error if r == -1?
}
};
+
+exports.ChildProcess = ChildProcess;
Something went wrong with that request. Please try again.