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

lib,doc: return boolean from child.send() #3516

Closed
wants to merge 5 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 25, 2015

The documentation indicates that child.send() returns a boolean but it
has returned undefined at least since io.js v1. This PR makes it so it returns a
boolean per the (slightly updated) documentation.

@Trott Trott added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. labels Oct 25, 2015
@Trott
Copy link
Member Author

Trott commented Oct 25, 2015

@Fishrock123
Copy link
Contributor

Seems fine to me, is this Major? I can't imagine anyone doing anything other than if (returnVal) checking it.

@Trott
Copy link
Member Author

Trott commented Oct 27, 2015

A more cautious alternative is #3518. While this one updates the code to conform to the documentation, that one updates the documentation to conform to the code. Which is the right path? ¯\_(ツ)_/¯

@Trott
Copy link
Member Author

Trott commented Oct 28, 2015

/cc @rvagg @jasnell for help with semver determination

@rvagg rvagg added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 28, 2015
@rvagg
Copy link
Member

rvagg commented Oct 28, 2015

semver-minor IMO

@@ -577,7 +577,7 @@ function setupChannel(target, channel) {
handle: null,
message: message,
});
return;
return this._handleQueue.length < (65536 * 2);
Copy link
Member

Choose a reason for hiding this comment

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

128k queued messages is perhaps a bit much. Maybe return false when the length is > 1?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. If the threshold is 1, I should probably add a test for that if it's possible to induce it without a ton of load or anything.

The documentation indicates that child.send() returns a boolean but it
has returned undefinined at least since io.js v1. It now returns a
boolean per the (slightly updated) documentation.
@jasnell
Copy link
Member

jasnell commented Oct 28, 2015

I'd agree with semver-minor as @rvagg indicates (which, of course, takes it out of the v4.x queue)

@@ -577,7 +577,7 @@ function setupChannel(target, channel) {
handle: null,
message: message,
});
return;
return this._handleQueue.length < 1;
Copy link
Member

Choose a reason for hiding this comment

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

This is logically always false. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Doh! Fixed.

@bnoordhuis
Copy link
Member

LGTM

@Trott
Copy link
Member Author

Trott commented Oct 28, 2015

I'll land this and close #3518 in about six hours unless there's an objection.

Trott added a commit that referenced this pull request Oct 28, 2015
The documentation indicates that child.send() returns a boolean but it
has returned undefinined at since v0.12.0. It now returns a boolean per
the (slightly updated) documentation.

PR-URL: #3516
Reviewed-By: Ben Noordhuis <[email protected]>
@Trott
Copy link
Member Author

Trott commented Oct 28, 2015

Landed in cdcf00a

@Trott Trott closed this Oct 28, 2015
rvagg pushed a commit to rvagg/io.js that referenced this pull request Oct 29, 2015
The documentation indicates that child.send() returns a boolean but it
has returned undefinined at since v0.12.0. It now returns a boolean per
the (slightly updated) documentation.

PR-URL: nodejs#3516
Reviewed-By: Ben Noordhuis <[email protected]>
@rvagg rvagg mentioned this pull request Oct 29, 2015
@cjihrig cjihrig mentioned this pull request Jul 12, 2016
3 tasks
@MylesBorins
Copy link
Contributor

@nodejs/lts should we consider including this in v4.5.0?

@MylesBorins MylesBorins mentioned this pull request Jul 14, 2016
4 tasks
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Jul 14, 2016
The documentation indicates that child.send() returns a boolean but it
has returned undefinined at since v0.12.0. It now returns a boolean per
the (slightly updated) documentation.

PR-URL: nodejs#3516
Reviewed-By: Ben Noordhuis <[email protected]>
@rvagg
Copy link
Member

rvagg commented Jul 18, 2016

a soft -1 on LTS for this from me @thealphanerd

@MylesBorins
Copy link
Contributor

thanks @rvagg would you be willing to drop the sentiment in #7739

@Trott Trott deleted the child-send branch January 13, 2022 22:29
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. doc Issues and PRs related to the documentations. 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.

6 participants