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: return boolean from child.send() #3577

Closed
wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 28, 2015

Previous change reinstated returning boolean from child.send() but
missed one instance where undefined might be returned instead.

R=@bnoordhuis

@Trott Trott added the child_process Issues and PRs related to the child_process subsystem. label Oct 28, 2015
@Trott
Copy link
Member Author

Trott commented Oct 29, 2015

@Trott Trott changed the title lib,doc: return boolean from child.send() lib: return boolean from child.send() Oct 29, 2015

const rv = n.send({ hello: 'world' });
assert.strictEqual(rv, true);

const s = spawn(process.execPath, [emptyFile],
{ stdio: ['pipe', 'pipe', 'pipe', 'ipc'] });
Copy link
Member

Choose a reason for hiding this comment

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

Nit: argument should line up.

@bnoordhuis
Copy link
Member

LGTM with style nit.

Previous change reinstated returning boolean from child.send() but
missed one instance where undefined might be returned instead.
@Trott
Copy link
Member Author

Trott commented Oct 29, 2015

Re-running CI after fixing up style nit: https://ci.nodejs.org/job/node-test-pull-request/646/

@bnoordhuis
Copy link
Member

I suppose you can't be too careful but I don't think anyone would yell at you if you added the spaces and landed it.

@Trott
Copy link
Member Author

Trott commented Oct 30, 2015

Landed in cf0130d

@Trott Trott closed this Oct 30, 2015
Trott added a commit that referenced this pull request Oct 30, 2015
Previous change reinstated returning boolean from child.send() but
missed one instance where undefined might be returned instead.

PR-URL: #3577
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@rvagg
Copy link
Member

rvagg commented Oct 30, 2015

@Trott I'm thinking this could be semver-minor, whaddya reckon?

@Trott
Copy link
Member Author

Trott commented Oct 30, 2015

@rvagg I personally wouldn't think so, but if there's any doubt, then yeah, let's be conservative and go with semver-minor.

Trott added a commit that referenced this pull request Nov 7, 2015
Previous change reinstated returning boolean from child.send() but
missed one instance where undefined might be returned instead.

PR-URL: #3577
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Nov 10, 2015
Notable changes:

* **child_process**: child.send() now properly returns a boolean like
the docs suggest (Rich Trott) nodejs#3577.
* **http_parser**: update http-parser to 2.6.0 from 2.5.0 (James M
Snell) nodejs#3569.
  - Now supports the following HTTP methods: `LINK`, `UNLINK`, `BIND`,
`REBIND`, `UNBIND`.
  - Also added ACL and IPv6 Zone ID support.
* **npm**: upgrade npm to 3.3.12 from v3.3.6 (Rebecca Turner)
nodejs#3685.
  - See the release notes for
    https://github.com/npm/npm/releases/tag/v3.3.7,
    https://github.com/npm/npm/releases/tag/v3.3.8,
    https://github.com/npm/npm/releases/tag/v3.3.9,
    https://github.com/npm/npm/releases/tag/v3.3.10,
    https://github.com/npm/npm/releases/tag/v3.3.11, and
    https://github.com/npm/npm/releases/tag/v3.3.12 for more details.
* **repl**: The REPL no longer crashes if the persistent history file
cannot be opened (Evan Lucas) nodejs#3630.
@Fishrock123 Fishrock123 mentioned this pull request Nov 11, 2015
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Nov 17, 2015
Notable changes:

* **child_process**: child.send() now properly returns a boolean like
the docs suggest (Rich Trott) nodejs#3577.
* **http_parser**: update http-parser to 2.6.0 from 2.5.0 (James M
Snell) nodejs#3569.
  - Now supports the following HTTP methods: `LINK`, `UNLINK`, `BIND`,
`REBIND`, `UNBIND`.
  - Also added ACL and IPv6 Zone ID support.
* **npm**: upgrade npm to 3.3.12 from v3.3.6 (Rebecca Turner)
nodejs#3685.
  - See the release notes for
    https://github.com/npm/npm/releases/tag/v3.3.7,
    https://github.com/npm/npm/releases/tag/v3.3.8,
    https://github.com/npm/npm/releases/tag/v3.3.9,
    https://github.com/npm/npm/releases/tag/v3.3.10,
    https://github.com/npm/npm/releases/tag/v3.3.11, and
    https://github.com/npm/npm/releases/tag/v3.3.12 for more details.
* **repl**: The REPL no longer crashes if the persistent history file
cannot be opened (Evan Lucas) nodejs#3630.

PR-URL: nodejs#3736
Fishrock123 added a commit that referenced this pull request Nov 17, 2015
Notable changes:

* buffer: The `noAssert` option for many buffer functions will now silently drop invalid write values rather than crashing (P.S.V.R) #3767.
  - This makes the behavior match what the docs suggest.
* child_process: `child.send()` now properly returns a boolean like
the docs suggest (Rich Trott) #3577.
* doc: All of the API docs have been re-ordered so as to read in alphabetical order (Tristian Flanagan) #3662.
* http_parser: update http-parser to 2.6.0 from 2.5.0 (James M
Snell) #3569.
  - Now supports the following HTTP methods: `LINK`, `UNLINK`, `BIND`,
`REBIND`, `UNBIND`.
  - Also added ACL and IPv6 Zone ID support.
* npm: upgrade npm to 3.3.12 from v3.3.6 (Rebecca Turner)
#3685.
  - See the release notes for
    https://github.com/npm/npm/releases/tag/v3.3.7,
    https://github.com/npm/npm/releases/tag/v3.3.8,
    https://github.com/npm/npm/releases/tag/v3.3.9,
    https://github.com/npm/npm/releases/tag/v3.3.10,
    https://github.com/npm/npm/releases/tag/v3.3.11, and
    https://github.com/npm/npm/releases/tag/v3.3.12 for more details.
* repl: The REPL no longer crashes if the persistent history file
cannot be opened (Evan Lucas) #3630.
* tls: The default `sessionIdContext` now uses SHA1 in FIPS mode rather than MD5 (Stefan Budeanu) #3755.
* v8: Added some more useful post-mortem data (Fedor Indutny) #3779.

PR-URL: #3736
@rvagg rvagg mentioned this pull request Dec 17, 2015
@MylesBorins
Copy link
Contributor

I just want to confirm that this is 100% being considered a patch before merging into LTS

/cc @rvagg @jasnell @Trott

@Trott
Copy link
Member Author

Trott commented Dec 30, 2015

@thealphanerd I consider it patch-level. But I wouldn't put up any resistance if someone else felt differently.

@rvagg
Copy link
Member

rvagg commented Dec 30, 2015

+1 for LTS @thealphanerd

@jasnell
Copy link
Member

jasnell commented Dec 30, 2015

+1 for LTS
On Dec 30, 2015 4:21 AM, "Rod Vagg" notifications@github.com wrote:

+1 for LTS @thealphanerd https://github.com/TheAlphaNerd


Reply to this email directly or view it on GitHub
#3577 (comment).

@MylesBorins
Copy link
Contributor

So I just attempted to merge this into LTS and got conflicts as we have not landed #3516

As that is not landing does it make sense to add this change?

/cc @Trott @jasnell @rvagg

@Trott
Copy link
Member Author

Trott commented Jan 7, 2016

This is basically a patch-fix to a previous semver minor, I guess it makes sense to not land it unless you are landing the semver minor patch first.

Does that make sense?

@MylesBorins
Copy link
Contributor

That was my thoughts exactly. I am removing the lts-watch label. Please feel free to audit and change this @jasnell

bbondy pushed a commit to brave/node that referenced this pull request Mar 13, 2016
Notable changes:

* buffer: The `noAssert` option for many buffer functions will now silently drop invalid write values rather than crashing (P.S.V.R) nodejs/node#3767.
  - This makes the behavior match what the docs suggest.
* child_process: `child.send()` now properly returns a boolean like
the docs suggest (Rich Trott) nodejs/node#3577.
* doc: All of the API docs have been re-ordered so as to read in alphabetical order (Tristian Flanagan) nodejs/node#3662.
* http_parser: update http-parser to 2.6.0 from 2.5.0 (James M
Snell) nodejs/node#3569.
  - Now supports the following HTTP methods: `LINK`, `UNLINK`, `BIND`,
`REBIND`, `UNBIND`.
  - Also added ACL and IPv6 Zone ID support.
* npm: upgrade npm to 3.3.12 from v3.3.6 (Rebecca Turner)
nodejs/node#3685.
  - See the release notes for
    https://github.com/npm/npm/releases/tag/v3.3.7,
    https://github.com/npm/npm/releases/tag/v3.3.8,
    https://github.com/npm/npm/releases/tag/v3.3.9,
    https://github.com/npm/npm/releases/tag/v3.3.10,
    https://github.com/npm/npm/releases/tag/v3.3.11, and
    https://github.com/npm/npm/releases/tag/v3.3.12 for more details.
* repl: The REPL no longer crashes if the persistent history file
cannot be opened (Evan Lucas) nodejs/node#3630.
* tls: The default `sessionIdContext` now uses SHA1 in FIPS mode rather than MD5 (Stefan Budeanu) nodejs/node#3755.
* v8: Added some more useful post-mortem data (Fedor Indutny) nodejs/node#3779.

PR-URL: nodejs/node#3736
bbondy pushed a commit to brave/node that referenced this pull request Mar 14, 2016
Notable changes:

* buffer: The `noAssert` option for many buffer functions will now silently drop invalid write values rather than crashing (P.S.V.R) nodejs/node#3767.
  - This makes the behavior match what the docs suggest.
* child_process: `child.send()` now properly returns a boolean like
the docs suggest (Rich Trott) nodejs/node#3577.
* doc: All of the API docs have been re-ordered so as to read in alphabetical order (Tristian Flanagan) nodejs/node#3662.
* http_parser: update http-parser to 2.6.0 from 2.5.0 (James M
Snell) nodejs/node#3569.
  - Now supports the following HTTP methods: `LINK`, `UNLINK`, `BIND`,
`REBIND`, `UNBIND`.
  - Also added ACL and IPv6 Zone ID support.
* npm: upgrade npm to 3.3.12 from v3.3.6 (Rebecca Turner)
nodejs/node#3685.
  - See the release notes for
    https://github.com/npm/npm/releases/tag/v3.3.7,
    https://github.com/npm/npm/releases/tag/v3.3.8,
    https://github.com/npm/npm/releases/tag/v3.3.9,
    https://github.com/npm/npm/releases/tag/v3.3.10,
    https://github.com/npm/npm/releases/tag/v3.3.11, and
    https://github.com/npm/npm/releases/tag/v3.3.12 for more details.
* repl: The REPL no longer crashes if the persistent history file
cannot be opened (Evan Lucas) nodejs/node#3630.
* tls: The default `sessionIdContext` now uses SHA1 in FIPS mode rather than MD5 (Stefan Budeanu) nodejs/node#3755.
* v8: Added some more useful post-mortem data (Fedor Indutny) nodejs/node#3779.

PR-URL: nodejs/node#3736
bbondy pushed a commit to brave/node that referenced this pull request Mar 14, 2016
Notable changes:

* buffer: The `noAssert` option for many buffer functions will now silently drop invalid write values rather than crashing (P.S.V.R) nodejs/node#3767.
  - This makes the behavior match what the docs suggest.
* child_process: `child.send()` now properly returns a boolean like
the docs suggest (Rich Trott) nodejs/node#3577.
* doc: All of the API docs have been re-ordered so as to read in alphabetical order (Tristian Flanagan) nodejs/node#3662.
* http_parser: update http-parser to 2.6.0 from 2.5.0 (James M
Snell) nodejs/node#3569.
  - Now supports the following HTTP methods: `LINK`, `UNLINK`, `BIND`,
`REBIND`, `UNBIND`.
  - Also added ACL and IPv6 Zone ID support.
* npm: upgrade npm to 3.3.12 from v3.3.6 (Rebecca Turner)
nodejs/node#3685.
  - See the release notes for
    https://github.com/npm/npm/releases/tag/v3.3.7,
    https://github.com/npm/npm/releases/tag/v3.3.8,
    https://github.com/npm/npm/releases/tag/v3.3.9,
    https://github.com/npm/npm/releases/tag/v3.3.10,
    https://github.com/npm/npm/releases/tag/v3.3.11, and
    https://github.com/npm/npm/releases/tag/v3.3.12 for more details.
* repl: The REPL no longer crashes if the persistent history file
cannot be opened (Evan Lucas) nodejs/node#3630.
* tls: The default `sessionIdContext` now uses SHA1 in FIPS mode rather than MD5 (Stefan Budeanu) nodejs/node#3755.
* v8: Added some more useful post-mortem data (Fedor Indutny) nodejs/node#3779.

PR-URL: nodejs/node#3736
bbondy pushed a commit to brave/node that referenced this pull request Mar 15, 2016
Notable changes:

* buffer: The `noAssert` option for many buffer functions will now silently drop invalid write values rather than crashing (P.S.V.R) nodejs/node#3767.
  - This makes the behavior match what the docs suggest.
* child_process: `child.send()` now properly returns a boolean like
the docs suggest (Rich Trott) nodejs/node#3577.
* doc: All of the API docs have been re-ordered so as to read in alphabetical order (Tristian Flanagan) nodejs/node#3662.
* http_parser: update http-parser to 2.6.0 from 2.5.0 (James M
Snell) nodejs/node#3569.
  - Now supports the following HTTP methods: `LINK`, `UNLINK`, `BIND`,
`REBIND`, `UNBIND`.
  - Also added ACL and IPv6 Zone ID support.
* npm: upgrade npm to 3.3.12 from v3.3.6 (Rebecca Turner)
nodejs/node#3685.
  - See the release notes for
    https://github.com/npm/npm/releases/tag/v3.3.7,
    https://github.com/npm/npm/releases/tag/v3.3.8,
    https://github.com/npm/npm/releases/tag/v3.3.9,
    https://github.com/npm/npm/releases/tag/v3.3.10,
    https://github.com/npm/npm/releases/tag/v3.3.11, and
    https://github.com/npm/npm/releases/tag/v3.3.12 for more details.
* repl: The REPL no longer crashes if the persistent history file
cannot be opened (Evan Lucas) nodejs/node#3630.
* tls: The default `sessionIdContext` now uses SHA1 in FIPS mode rather than MD5 (Stefan Budeanu) nodejs/node#3755.
* v8: Added some more useful post-mortem data (Fedor Indutny) nodejs/node#3779.

PR-URL: nodejs/node#3736
@mscdex mscdex mentioned this pull request Jul 14, 2016
4 tasks
@Trott Trott deleted the child-send-again 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants