errors, child_process: migrate to using internal/errors by jasnell · Pull Request #11300 · nodejs/node

jasnell · 2017-02-10T20:10:25Z

Ref: #11273

Semver-major because error messages are changed.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

errors, child_process

jasnell · 2017-03-22T21:31:25Z

@nodejs/ctc ... can I please get a review on this?

jasnell · 2017-03-22T21:32:08Z

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

jasnell · 2017-03-22T21:56:51Z

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

targos

almost there

targos · 2017-03-23T15:36:50Z

doc/api/errors.md

missing space

targos · 2017-03-23T15:38:52Z

doc/api/errors.md

nit: passed in?

targos · 2017-03-23T15:40:42Z

doc/api/errors.md

nit: communication

targos · 2017-03-23T15:42:30Z

doc/api/errors.md

maybe ERR_IPC_CHANNEL_CLOSED?

targos · 2017-03-23T15:43:05Z

doc/api/errors.md

ditto. ERR_IPC_INVALID_HANDLE_TYPE

targos · 2017-03-23T15:48:37Z

lib/internal/errors.js

If I read it correctly, this would print The "options" argument must be type Object. It looks better to me as The "options" argument must be of type Object

~~It would be nice to have some tests for this function~~ OK, tests are in #11294

targos · 2017-03-23T15:51:54Z

lib/internal/child_process.js

The name argument is missing

targos · 2017-03-23T15:52:12Z

lib/internal/child_process.js

mhdawson

Generally looks ok to me, but needs updates requested by @targos

jasnell · 2017-04-20T23:05:50Z

@targos @mhdawson ... this is unblocked now. PR is updated. PTAL

jasnell · 2017-04-21T18:10:59Z

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

targos · 2017-04-24T14:28:17Z

lib/internal/errors.js

Why did you remove this check? (The test for it is removed in the second commit btw)

Because of issues in the loading order of the errors.js module relative to assert.js on startup. There is a circular dependency that happens that causes the util.js used within assert.js to fail depending on when assert is loaded. I could replace this with a simple throw rather than the call to assert if you'd be more comfortable with that.

I'm fine with removing it. I was just curious. The risk of reusing the same code is low if we keep them together and in alphabetical order.

targos · 2017-04-24T14:38:33Z

lib/internal/errors.js

unnecessary new line

whoops! fixed

targos · 2017-04-24T14:39:17Z

lib/internal/errors.js

please keep the codes in alphabetical order relative to the existing ones.

targos · 2017-04-24T14:40:36Z

test/parallel/test-internal-errors.js

please bundle this change with the same commit that removed the error

jasnell · 2017-04-24T16:27:43Z

@targos @mhdawson ... PTAL

jasnell · 2017-04-24T16:29:28Z

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

targos

LGTM. This needs a rebase.

jasnell · 2017-04-26T16:38:31Z

Rebased. Ping @mhdawson

mhdawson

LGTM

jasnell · 2017-04-27T19:51:37Z

Thank you @mhdawson and @targos. Rebased again and new CI before landing: https://ci.nodejs.org/job/node-test-pull-request/7715/

Use of assert must be lazy to allow errors to be used early before the process is completely set up

Use of assert must be lazy to allow errors to be used early before the process is completely set up PR-URL: #11300 Ref: #11273 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: #11300 Ref: #11273 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

jasnell · 2017-04-27T22:46:19Z

Landed in f0b7025 and 7632761

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 10, 2017

nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Feb 10, 2017

jasnell mentioned this pull request Feb 10, 2017

Tracking Issue: Migrate errors to internal/errors.js #11273

Closed

80 tasks

This was referenced Feb 13, 2017

errors, path: migrate to use internal/errors.js #11319

Closed

internal/util: use internal/errors.js #11301

Closed

jasnell force-pushed the internal-errors-3 branch from c05e185 to 7839c5b Compare March 22, 2017 21:30

jasnell added the ctc-review label Mar 22, 2017

jasnell force-pushed the internal-errors-3 branch from 7839c5b to d3e735c Compare March 22, 2017 21:56

targos requested changes Mar 23, 2017

View reviewed changes

targos mentioned this pull request Mar 23, 2017

errors, process: port internal/process errors to internal/errors #11294

Closed

4 tasks

Trott mentioned this pull request Mar 26, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-03-29 nodejs/CTC#85

Closed

Trott mentioned this pull request Apr 3, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-04-05 nodejs/CTC#93

Closed

mhdawson reviewed Apr 5, 2017

View reviewed changes

jasnell added the blocked PRs that are blocked by other issues or PRs. label Apr 5, 2017

Trott mentioned this pull request Apr 9, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-04-12 nodejs/CTC#98

Closed

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

Trott mentioned this pull request Apr 17, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-04-19 nodejs/CTC#105

Closed

jasnell force-pushed the internal-errors-3 branch 2 times, most recently from e08db62 to 9470c7f Compare April 20, 2017 23:01

jasnell removed the blocked PRs that are blocked by other issues or PRs. label Apr 20, 2017

jasnell force-pushed the internal-errors-3 branch from 9470c7f to 0dd4e33 Compare April 21, 2017 18:29

Trott mentioned this pull request Apr 24, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-04-26 nodejs/CTC#111

Closed

targos reviewed Apr 24, 2017

View reviewed changes

targos requested changes Apr 24, 2017

View reviewed changes

jasnell force-pushed the internal-errors-3 branch from 0dd4e33 to d7a1a80 Compare April 24, 2017 16:25

targos approved these changes Apr 26, 2017

View reviewed changes

jasnell force-pushed the internal-errors-3 branch from d7a1a80 to 55be06e Compare April 26, 2017 16:37

mhdawson approved these changes Apr 27, 2017

View reviewed changes

jasnell force-pushed the internal-errors-3 branch from 55be06e to 6f5a9ab Compare April 27, 2017 19:50

jasnell added 2 commits April 27, 2017 15:36

errors: use lazy assert to avoid issues on startup

896e8db

Use of assert must be lazy to allow errors to be used early before the process is completely set up

errors, child_process: migrate to using internal/errors

14cacc6

jasnell force-pushed the internal-errors-3 branch from 6f5a9ab to 14cacc6 Compare April 27, 2017 22:43

jasnell added a commit that referenced this pull request Apr 27, 2017

errors, child_process: migrate to using internal/errors

7632761

PR-URL: #11300 Ref: #11273 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

jasnell closed this Apr 27, 2017

Trott removed the ctc-review label May 1, 2017

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

lundibundi mentioned this pull request Sep 5, 2020

lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_ENCODING #34682

Closed

4 tasks

Uh oh!

Conversation

jasnell commented Feb 10, 2017

Checklist

Affected core subsystem(s)

Uh oh!

jasnell commented Mar 22, 2017

Uh oh!

jasnell commented Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Mar 22, 2017

Uh oh!

targos 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!

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!

targos Mar 23, 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!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 20, 2017

Uh oh!

jasnell commented Apr 21, 2017

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!

jasnell commented Apr 24, 2017

Uh oh!

jasnell commented Apr 24, 2017

Uh oh!

targos left a comment • 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!

jasnell commented Apr 26, 2017

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 27, 2017

Uh oh!

jasnell commented Apr 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

jasnell commented Mar 22, 2017 •

edited

Loading

targos Mar 23, 2017 •

edited

Loading

targos left a comment •

edited

Loading