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

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

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

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

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

almost there

missing space

nit: passed in?

nit: communication

maybe ERR_IPC_CHANNEL_CLOSED?

ditto. ERR_IPC_INVALID_HANDLE_TYPE

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

The name argument is missing

ditto

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

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

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

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.

unnecessary new line

whoops! fixed

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

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

@targos @mhdawson ... PTAL

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

LGTM. This needs a rebase.

Rebased. Ping @mhdawson

LGTM

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>

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 suggested 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 suggested 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

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

Uh oh!

Uh oh!

errors, child_process: migrate to using internal/errors - #11300

errors, child_process: migrate to using internal/errors#11300jasnell wants to merge 2 commits intonodejs:masternodejs/node:masterfrom jasnell:internal-errors-3jasnell/node:internal-errors-3Copy head branch name to clipboard

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

errors, child_process: migrate to using internal/errors#11300
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:internal-errors-3

jasnell commented Mar 22, 2017 •
edited
Loading

targos Mar 23, 2017 •
edited
Loading

targos left a comment •
edited
Loading