feat: supports serialization of options#97

waitingsong · 2020-02-13T12:16:14Z

ref: https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V12.md#advanced-serialization-for-ipc

Checklist

npm test passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

Description of change

ref: https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V12.md#advanced-serialization-for-ipc

waitingsong · 2020-02-13T12:17:11Z

Node.js 12.16 支持高级序列化方式
https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V12.md#advanced-serialization-for-ipc

codecov · 2020-02-13T12:44:08Z

Codecov Report

Merging #97 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@ Coverage Diff @@## master #97 +/- ## ========================================== + Coverage 98.82% 98.83% +0.01%  ========================================== Files 8 8 Lines 511 517 +6 ========================================== + Hits 505 511 +6  Misses 6 6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 423726d...1249d64. Read the comment docs.

waitingsong · 2020-02-13T13:30:32Z

serialization 在 Node.js < 12.16 时可以传递任何值，不会异常。在 >= 12.16 时非 jsonadvanced 值会抛异常。

另，是否需要在 debug 模式时默认设置为 advanced 值？
或者当 >= 12.16.0 时全部默认 'advanced'？

ref: eggjs/cluster#97

hyj1991 · 2020-02-14T01:38:24Z

README.md

+| require |`Array\|String`| will inject into worker/agent process |
+| pidFile |`String`| will save master pid to this file |
+|[serialization]| `json|advanced` | default: 'json'. 'advanced' requires Node.js >= 10.16.0 |



这里是 12.16.0 吧？

另，是否需要在 debug 模式时默认设置为 advanced 值？
或者当 >= 12.16.0 时全部默认 'advanced'？
我看了下下面的相关讨论：nodejs/node#30162 (comment), 看起来 advanced 并不是所有场景下都是最优解，所以这里确实应该 follow 上游的设置，默认为 'json'，但是在框架里在适当的版本暴露可配置选项。
我也是如此考虑：随着版本更新性能提升及可靠性得到进一步验证，可能在未来某个版本默认改为 advanced。
所以跟随 node.js 默认设置（json），代码里面不做默认值设置，让有需要的用户自行决定传入参数控制。

README.md

hyj1991 · 2020-02-14T01:42:40Z

lib/master.js

 if(this.options.isDebug)opt.execArgv=process.execArgv.concat([`--${semver.gte(process.version,'8.0.0') ? 'inspect' : 'debug'}-port=${debugPort}`]);

+/* istanbul ignore next */
+if(semver.gte(process.version,'12.16.0')){


这里的判断有一个问题，'v13.0.0' 也是 >= 12.16.0，但是这个版本 patch 了此功能么

对于 12 版本是 >= 12.16.0，对于 13 版本是 >= 13.2.0。
代码已更新判断条件。

lib/utils/options.js

hyj1991

感谢您关于 v12.16.0 更新的支持更多 IPC 序列/反序列化的配置参数 PR，以上是我的一些对于 PR 代码的修改建议，期望可以一起针对修改/不修改的合理性进行讨论 :)

hyj1991 · 2020-02-14T02:02:23Z

另，是否需要在 debug 模式时默认设置为 advanced 值？
或者当 >= 12.16.0 时全部默认 'advanced'？

我看了下下面的相关讨论：nodejs/node#30162 (comment), 看起来 advanced 并不是所有场景下都是最优解，所以这里确实应该 follow 上游的设置，默认为 'json'，但是在框架里在适当的版本暴露可配置选项。

atian25 · 2020-02-14T02:28:10Z

如果这个未来还会变化的话，那目前能否先通过 NODE_OPTIONS ENV 的方式来使用就好了？

>= v12.16.0 < v13.0.0, or >= v13.2.0

waitingsong · 2020-02-14T02:31:29Z

NODE_OPTIONS ENV

这个变量设置是否影响 forkAgentWorker() 对 agent 进程的派生。

atian25 · 2020-02-14T03:07:15Z

NODE_OPTIONS ENV
这个变量设置是否影响 forkAgentWorker() 对 agent 进程的派生。

影响的，fork 默认是继承 env 的。可以试验下

waitingsong · 2020-02-14T03:43:24Z

NODE_OPTIONS ENV
这个变量设置是否影响 forkAgentWorker() 对 agent 进程的派生。
影响的，fork 默认是继承 env 的。可以试验下

就是说不修改代码，通过设置 ENV 也能起效果。那我关闭这个 PR 了哟？

atian25 · 2020-02-14T03:44:13Z

NODE_OPTIONS ENV
这个变量设置是否影响 forkAgentWorker() 对 agent 进程的派生。
影响的，fork 默认是继承 env 的。可以试验下
就是说不修改代码，通过设置 ENV 也能起效果。那我关闭这个 PR 了哟？

我理解是这样的，你实验下看看

waitingsong · 2020-02-14T03:46:38Z

NODE_OPTIONS ENV
这个变量设置是否影响 forkAgentWorker() 对 agent 进程的派生。
影响的，fork 默认是继承 env 的。可以试验下
就是说不修改代码，通过设置 ENV 也能起效果。那我关闭这个 PR 了哟？
我理解是这样的，你实验下看看

那我测试下。

hyj1991 · 2020-02-18T03:51:36Z

所以这个 PR 还有下文不。。。

waitingsong · 2020-02-18T11:41:53Z

所以这个 PR 还有下文不。。。

么时间测试啊，我先关掉。

feat: supports serialization of options
59e8612
ref: https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V12.md#advanced-serialization-for-ipc

atian25 requested a review from hyj1991 February 13, 2020 12:22

chore: fix style
8aa9fc2

chore: move option.serialization of validation into utils/options.js
23fc7b3

waitingsong force-pushed the child_process branch from 96f73a0 to 23fc7b3Compare February 13, 2020 12:52

waitingsong force-pushed the child_process branch from 120bebe to 0eba33bCompare February 13, 2020 13:34

waitingsong added a commit to waitingsong/egg that referenced this pull request Feb 13, 2020
feat(typings): add ClusterOptions['serialization']
3a0958f
ref: eggjs/cluster#97

waitingsong mentioned this pull request Feb 13, 2020
feat(typings): add ClusterOptions['serialization'] eggjs/egg#4182
Closed
4 tasks

waitingsong force-pushed the child_process branch from 0eba33b to e9d9069Compare February 13, 2020 14:05

refactor: update validation of options.serialization
9d9f33a

waitingsong force-pushed the child_process branch from e9d9069 to 9d9f33aCompare February 13, 2020 14:57

chore: update parameter comments of class Master
9727bf3

hyj1991 reviewed Feb 14, 2020
View reviewed changes

README.md Outdated Show resolvedHide resolved

hyj1991 reviewed Feb 14, 2020
View reviewed changes

lib/utils/options.js Outdated Show resolvedHide resolved

chore: clean README.md
097dd0a

hyj1991 requested changes Feb 14, 2020
View reviewed changes

feat: update Node.js version requirement for serialization
1249d64
>= v12.16.0 < v13.0.0, or >= v13.2.0

waitingsong force-pushed the child_process branch from 4179e83 to 1249d64Compare February 14, 2020 02:28

waitingsong closed this Feb 18, 2020

feat: supports serialization of options#97

feat: supports serialization of options #97

Uh oh!

Conversation

waitingsong commented Feb 13, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

waitingsong commented Feb 13, 2020

Uh oh!

codecovbot commented Feb 13, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

waitingsong commented Feb 13, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hyj1991Feb 14, 2020

Choose a reason for hiding this comment

Uh oh!

waitingsongFeb 14, 2020• 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!

Uh oh!

hyj1991Feb 14, 2020

Choose a reason for hiding this comment

Uh oh!

waitingsongFeb 14, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hyj1991 left a comment

Choose a reason for hiding this comment

Uh oh!

hyj1991 commented Feb 14, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atian25 commented Feb 14, 2020

Uh oh!

waitingsong commented Feb 14, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atian25 commented Feb 14, 2020

Uh oh!

waitingsong commented Feb 14, 2020

Uh oh!

atian25 commented Feb 14, 2020

Uh oh!

waitingsong commented Feb 14, 2020

Uh oh!

hyj1991 commented Feb 18, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

waitingsong commented Feb 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

waitingsong commented Feb 13, 2020•
edited
Loading

codecovbot commented Feb 13, 2020•
edited
Loading

waitingsong commented Feb 13, 2020•
edited
Loading

waitingsongFeb 14, 2020•
edited
Loading

hyj1991 commented Feb 14, 2020•
edited
Loading

waitingsong commented Feb 14, 2020•
edited
Loading

hyj1991 commented Feb 18, 2020•
edited
Loading