Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

child_process: expose ChildProcess constructor #1760

Merged
merged 2 commits into from
May 28, 2015

Conversation

evanlucas
Copy link
Contributor

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

Related: #1751

Definitely needs more tests for the internal modules.

var common = require('../common');
var child_process = require('child_process');

assert.equal(typeof child_process.ChildProcess, 'function');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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.

@@ -0,0 +1,108 @@
'use strict';

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evanlucas add objectLiteralShorthandProperties: true to ecmaFeatures

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

score. Thanks!!!

@chrisdickinson
Copy link
Contributor

This looks great! Thanks for putting it together.

@mscdex mscdex added the child_process Issues and PRs related to the child_process subsystem. label May 21, 2015
sindresorhus added a commit to sindresorhus/child-process-ctor that referenced this pull request May 22, 2015
@evanlucas evanlucas force-pushed the cp_internal branch 2 times, most recently from 822d38e to ada79e5 Compare May 22, 2015 14:53
@evanlucas
Copy link
Contributor Author

Ok, added more to the tests

@evanlucas
Copy link
Contributor Author

@evanlucas
Copy link
Contributor Author

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
Copy link
Contributor

@evanlucas these look suspiciously like the ones that were failing before. Maybe start off a CI off master to be sure.

@evanlucas
Copy link
Contributor Author

Yea, seeing if I can reproduce locally now

@silverwind
Copy link
Contributor

@evanlucas
Copy link
Contributor Author

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
Copy link
Contributor

@evanlucas
Copy link
Contributor Author

Nice, CI seems to like it

@chrisdickinson
Copy link
Contributor

This LGTM.

Required to make linting pass for using object literal
shorthand properties.

PR-URL: nodejs#1760
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
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 <christopher.s.dickinson@gmail.com>
@evanlucas evanlucas merged commit a77c330 into nodejs:master May 28, 2015
@evanlucas evanlucas deleted the cp_internal branch May 28, 2015 14:39
@Fishrock123 Fishrock123 added the semver-minor PRs that contain new features and should be released in the next minor version. label May 28, 2015
@evanlucas
Copy link
Contributor Author

Thanks. Landed in fbd2b59 and a77c330

@isaacs
Copy link
Contributor

isaacs commented May 28, 2015

Awesome stuff. Thanks!

andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
Required to make linting pass for using object literal
shorthand properties.

PR-URL: nodejs/node#1760
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
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 <christopher.s.dickinson@gmail.com>
@eljefedelrodeodeljefe
Copy link
Contributor

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 require('child_process').ChildProcess. In the future please evaluate more carefully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants