child_process: expose ChildProcess constructor#1760
Uh oh!
There was an error while loading. Please reload this page.
Conversation
There was a problem hiding this comment.
we should flesh out this test file a bit – make sure we can instantiate it, call spawn on it, and call kill on it.
evanlucas commented May 21, 2015
Absolutely. Definitely needs more tests. Wanted to get the initial implementation up to make sure this direction was ok. I'll get more pushed up tonight. |
There was a problem hiding this comment.
I'd like it if we could move the exports up here, a la:
module.exports={SocketListSend, SocketListReceive}(We have shorthand object literals now, so we can take advantage of that!)
There was a problem hiding this comment.
Strange, keep getting a lint error when using this. https://gist.github.com/evanlucas/0920f2b5cd90340cca27
Any ideas @silverwind? I've tried adding in an object-shorthand rule, but it didn't seem to do anything
There was a problem hiding this comment.
@evanlucas add objectLiteralShorthandProperties: true to ecmaFeatures
chrisdickinson commented May 21, 2015
This looks great! Thanks for putting it together. |
mscdex added the child_process 
evanlucasforce-pushed the cp_internal branch 2 times, most recently from 822d38e to ada79e5Compareevanlucas commented May 22, 2015
Ok, added more to the tests |
evanlucas commented May 22, 2015
evanlucas commented May 23, 2015
Ok, I ran another CI run to see if it was this PR causing issues. https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/703/ I am not sure if it is or not. Seems to be some failures on win2008r2 still |
silverwind commented May 23, 2015
@evanlucas these look suspiciously like the ones that were failing before. Maybe start off a CI off master to be sure. |
evanlucas commented May 23, 2015
Yea, seeing if I can reproduce locally now |
silverwind commented May 23, 2015
CI off master to compare: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/704/ |
evanlucas commented May 24, 2015
Tests passed locally on windows 7 for me. I did get a dialog asking to allow access on one of the tests (not sure which one though) |
silverwind commented May 24, 2015
#1777 (comment) explains the win2008 failures. New CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/708/ |
evanlucas commented May 26, 2015
Nice, CI seems to like it |
chrisdickinson commented May 28, 2015
This LGTM. |
Required to make linting pass for using object literal shorthand properties. PR-URL: nodejs#1760 Reviewed-By: Chris Dickinson <[email protected]>
Creates two new internal modules (child_process and socket_list) for better readability. Exposes the ChildProcess constructor from the child_process module so one can now `require(‘child_process’).ChildProcess` Fixes: nodejs#1751 PR-URL: nodejs#1760 Reviewed-By: Chris Dickinson <[email protected]>
Fishrock123 added the semver-minor evanlucas commented May 28, 2015
isaacs commented May 28, 2015
Awesome stuff. Thanks! |
Required to make linting pass for using object literal shorthand properties. PR-URL: nodejs/node#1760 Reviewed-By: Chris Dickinson <[email protected]>
Creates two new internal modules (child_process and socket_list) for better readability. Exposes the ChildProcess constructor from the child_process module so one can now `require(‘child_process’).ChildProcess` Fixes: nodejs/node#1751 PR-URL: nodejs/node#1760 Reviewed-By: Chris Dickinson <[email protected]>
eljefedelrodeodeljefe commented May 6, 2016
In hindsight I would regard this as really bad decision just to have sugar for extension. This doesn't allow a decent rewrite of the child_process module which I will attempt now and wanted to do behind |
7 participants
Creates two new internal modules (child_process and socket_list) for
better readability.
Exposes the ChildProcess constructor from the child_process module so
one can now
require(‘child_process’).ChildProcessRelated: #1751
Definitely needs more tests for the internal modules.