FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib,doc: return boolean from child.send() by Trott · Pull Request #3516 · nodejs/node · GitHub

/ node Public

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

Closed
Trott wants to merge 5 commits into
nodejs:masterfrom
Trott:child-send
ConversationCommits (5)ChecksFiles changed
Closed

lib,doc: return boolean from child.send()#3516
Trott wants to merge 5 commits into
nodejs:masterfrom
Trott:child-send

Conversation

Trott commented Oct 25, 2015

Copy link
Copy Markdown
Member

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 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 commented Oct 25, 2015

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/610/

Trott mentioned this pull request Oct 25, 2015
Closed

Fishrock123 commented Oct 26, 2015

Copy link
Copy Markdown
Contributor

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

Trott commented Oct 27, 2015

Copy link
Copy Markdown
Member Author

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 commented Oct 28, 2015

Copy link
Copy Markdown
Member Author

/cc @rvagg @jasnell for help with semver determination

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

rvagg commented Oct 28, 2015

Copy link
Copy Markdown
Member

semver-minor IMO

bnoordhuis reviewed Oct 28, 2015
View reviewed changes
Comment thread lib/internal/child_process.js Outdated

bnoordhuis Oct 28, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Trott Oct 28, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Trott added 2 commits October 28, 2015 08:49
lib,doc: return boolean from child.send()
4456a33 
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.
fixup: it is going to be true
2779bfb

jasnell commented Oct 28, 2015

Copy link
Copy Markdown
Member

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

bnoordhuis reviewed Oct 28, 2015
View reviewed changes
Comment thread lib/internal/child_process.js Outdated

bnoordhuis Oct 28, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is logically always false. :-)

Trott Oct 28, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Doh! Fixed.

bnoordhuis commented Oct 28, 2015

Copy link
Copy Markdown
Member

LGTM

Trott commented Oct 28, 2015

Copy link
Copy Markdown
Member Author

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

bnoordhuis mentioned this pull request Oct 28, 2015
Closed
Trott added a commit that referenced this pull request Oct 28, 2015
lib,doc: return boolean from child.send()
cdcf00a 
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 <info@bnoordhuis.nl>

Trott commented Oct 28, 2015

Copy link
Copy Markdown
Member Author

Landed in cdcf00a

Trott closed this Oct 28, 2015
rvagg pushed a commit to rvagg/io.js that referenced this pull request Oct 29, 2015
lib,doc: return boolean from child.send()
f236b3a 
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 <info@bnoordhuis.nl>
rvagg mentioned this pull request Oct 29, 2015
Merged
MylesBorins mentioned this pull request Jan 6, 2016
Closed
cjihrig mentioned this pull request Jul 12, 2016
Merged
3 tasks

MylesBorins commented Jul 14, 2016

Copy link
Copy Markdown
Contributor

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

MylesBorins mentioned this pull request Jul 14, 2016
Closed
4 tasks
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Jul 14, 2016
lib,doc: return boolean from child.send()
1f05a05 
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 <info@bnoordhuis.nl>

rvagg commented Jul 18, 2016

Copy link
Copy Markdown
Member

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

MylesBorins commented Jul 18, 2016

Copy link
Copy Markdown
Contributor

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

Trott deleted the child-send branch January 13, 2022 22:29
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL