doc: note caveats in process message serialization #12963

joyeecheung · 2017-05-11T05:40:52Z

The message sent using process.send() goes through JSON
serialization and parsing, which could lead to surprising behaviors.
This commit elaborate a bit more on this and add a link to
the notes about these caveats in the ECMAScript specification.

Refs: #12497

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc, process, child_process

mscdex · 2017-05-11T05:55:38Z

doc/api/child_process.md

Perhaps simplify the link and adjust some of the wording here... for example:

*Note*: since the message is transmitted as JSON, not all values may be transferred as-is. See the documentation for [`JSON.stringify()`][] for more details.

Where the JSON.stringify() link goes to MDN instead (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#Description), which IMHO has a more readable description of when/how values are converted.

The MDN documentation doesn't mention the Nan/Infinity -> null thing which I think is one of the surprising behaviors..the link is not about the serialization process, but rather, the caveats, which are described in the notes of the ECMA spec (although I cannot find a direct anchor to those notes, so I use the link to the method instead).

Also the MDN documentation doesn't mention the circular reference error...:/ hmm

Nit: The comma after sent should be a period. See notes at... can be its own sentence.

mscdex · 2017-05-11T05:56:39Z

doc/api/child_process.md

I would just use the exact same text as suggested above for this part.

Nit: this -> This

mscdex · 2017-05-11T05:57:21Z

doc/api/process.md

Nit: since -> Since

mscdex · 2017-05-11T05:57:30Z

doc/api/process.md

Nit: this -> This

Trott · 2017-05-11T18:30:12Z

doc/api/process.md

Identical nit as above about comma and see notes.

cjihrig

I think the existing messaging (Note: this function uses [JSON.stringify()][] internally to serialize the message.) is good. The message should probably be moved to a better location in the send() docs though.

I don't think we should explain too much about how stringify() works, when we are linking to it already.

cjihrig · 2017-05-11T19:56:41Z

doc/api/child_process.md

JSON.stringify() is already in this list of links in both of these files.

sam-github · 2017-05-11T20:20:51Z

doc/api/process.md

please keep the refs sorted

BridgeAR · 2017-08-26T23:46:18Z

Ping @joyeecheung this needs a rebase and the comments still need to be addressed

Trott · 2017-08-27T02:06:37Z

Ping @joyeecheung this needs a rebase and the comments still need to be addressed

@joyeecheung I'm happy to push a commit for fixes for my nits if you'd like. Just let me know.

BridgeAR · 2017-09-19T22:20:55Z

Ping @Trott do you want to follow up on this? I would otherwise close this.

The message sent using process.send() goes through JSON serialization and parsing, which could lead to surprising behaviors. This commit elaborate a bit more on this and add a link to the notes about these caveats in the ECMAScript specification.

Trott · 2017-09-20T03:00:45Z

Ping @Trott do you want to follow up on this? I would otherwise close this.

I rebased, resolved conflicts, fixed all nits, and pushed back up to Joyee's branch. I think this is ready to land.

@cjihrig Can you review and clear your request for changes if this addresses your concerns? I'm not sure if the new wording I settled on is OK by you or not.

@joyeecheung Can you review and confirm that what I've done here meets your intentions?

@nodejs/documentation PTAL

BridgeAR · 2017-09-22T23:25:13Z

Landed in ba5a668

(I felt free to land this due to no response from @joyeecheung since a month)

The message sent using process.send() goes through JSON serialization and parsing, which could lead to surprising behaviors. This commit elaborate a bit more on this and add a link to the notes about these caveats in the ECMAScript specification. PR-URL: #12963 Refs: #12497 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

The message sent using process.send() goes through JSON serialization and parsing, which could lead to surprising behaviors. This commit elaborate a bit more on this and add a link to the notes about these caveats in the ECMAScript specification. PR-URL: nodejs/node#12963 Refs: nodejs/node#12497 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

The message sent using process.send() goes through JSON serialization and parsing, which could lead to surprising behaviors. This commit elaborate a bit more on this and add a link to the notes about these caveats in the ECMAScript specification. PR-URL: #12963 Refs: #12497 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 11, 2017

joyeecheung force-pushed the process-send-caveat branch from 8a3b2e1 to 6e47903 Compare May 11, 2017 05:43

joyeecheung added child_process Issues and PRs related to the child_process subsystem. process Issues and PRs related to the process subsystem. labels May 11, 2017

mscdex reviewed May 11, 2017

View reviewed changes

Trott reviewed May 11, 2017

View reviewed changes

doc/api/process.md Outdated

Copy link

Member

Trott May 11, 2017

Choose a reason for hiding this comment

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

Identical nit as above about comma and see notes.

cjihrig requested changes May 11, 2017

View reviewed changes

doc/api/child_process.md Outdated

Copy link

Contributor

cjihrig May 11, 2017

Choose a reason for hiding this comment

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

JSON.stringify() is already in this list of links in both of these files.

sam-github reviewed May 11, 2017

View reviewed changes

doc/api/process.md Outdated

Copy link

Contributor

sam-github May 11, 2017

Choose a reason for hiding this comment

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

please keep the refs sorted

Trott force-pushed the process-send-caveat branch from 6e47903 to 9d99868 Compare September 20, 2017 02:57

cjihrig approved these changes Sep 20, 2017

View reviewed changes

jasnell approved these changes Sep 20, 2017

View reviewed changes

vsemozhetbyt approved these changes Sep 20, 2017

View reviewed changes

BridgeAR approved these changes Sep 22, 2017

View reviewed changes

BridgeAR closed this Sep 22, 2017

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

Uh oh!

doc: note caveats in process message serialization #12963

doc: note caveats in process message serialization #12963

Uh oh!

Conversation

joyeecheung commented May 11, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung May 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex May 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Aug 26, 2017

Uh oh!

Trott commented Aug 27, 2017

Uh oh!

BridgeAR commented Sep 19, 2017

Uh oh!

Trott commented Sep 20, 2017

Uh oh!

BridgeAR commented Sep 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

joyeecheung May 11, 2017 •

edited

Loading

mscdex May 11, 2017 •

edited

Loading