errors, process: port internal/process errors to internal/errors by jasnell · Pull Request #11294 · nodejs/node · GitHub

Assign codes to the handful of errors reported by internal/process/*.js
Include documentation for the new error codes
Improve error messages
Improve test coverage for process.nextTick

Semver-major because this updates specific error messages and converts errors over to use the new internal/errors.js mechanism.

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, process

(Not sure which PR is the right place to ask, this one seems the oldest so I'll ask here)

Which one does the "type" here cover:

Primitive types (i.e. typeof types, doesn't differentiate Date and RegExp and stuff)
Builtin type tags(i.e. results of Object.prototype.toString.call(), differentiates Date and RegExp)
Class mismatch(i.e. instanceof types, differentiates Date and RegExp)

Also, some PRs use the typeof types as the expected type(e.g. "function"), some uses the class name/type tag(e.g. "Object"), we probably need to be a little bit consistent on this one.

This is not handled very consistently throughout the Node.js source. Sometimes the value itself is passed in, sometimes the typeof is used, etc. The goal here would be to start getting some consistency but it's hard to nail down exactly what that should be.

Got it, so we should focus on migrating the errors to use the new system with a reasonable code right now, and deal with message later, because after the migration it would be easier to change them anyway?

String(..) seems unnecessary here?

+1. It's already cast on line 103

@nodejs/ctc ... as a semver major this needs some review and signoff

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

@jasnell in the invalidArgType : what if the actual arg is null , the result will be . Received type object

Sigh, good point. I often forget that typeof null === 'object' which is really quite odd.

#11298 spells the full UNKNOWN, this should do the same (TBH I can not understand what UNK stands for at first glance...).

@nodejs/ctc ... PTAL... I'd like to get this landed so we can start getting through the others that are starting to stack up

ping @evanlucas, @addaleax, @cjihrig and @mhdawson

@nodejs/ctc ... I need at least one more CTC member sign off on this to proceed.

As I mentioned somewhere else (not sure where, there have been a bunch of these pull requests), I'm not thrilled with what the stack trace looks like after this change.

Which part are you unhappy with?

$ ./node --expose-internals
> new errors.Error('ERR_ASSERTION', 'something')
{ Error[ERR_ASSERTION]: something
    at repl:1:1
    at ContextifyScript.Script.runInThisContext (vm.js:44:33)
    at REPLServer.defaultEval (repl.js:239:29)
    at bound (domain.js:301:14)
    at REPLServer.runBound [as eval] (domain.js:314:12)
    at REPLServer.onLine (repl.js:433:10)
    at emitOne (events.js:120:20)
    at REPLServer.emit (events.js:210:7)
    at REPLServer.Interface._onLine (readline.js:262:10)
    at REPLServer.Interface._line (readline.js:611:8) [Symbol(code)]: 'ERR_ASSERTION' }

I just find it a little harder to read Error[ERR_ASSERTION]: in particular. As opposed to Error:

Do you have an alternative suggestion? Displaying the error code in the output is a significant reason why this allows us to avoid treating changes in the error message as semver-major... That is, it gives the user a stable, non-changing key they can use to search for information on the specific error. Putting the [ERR_ASSERTION] on the other side of the : requires changing the actual error message.

As I mentioned somewhere else (not sure where, there have been a bunch of these pull requests), I'm not thrilled with what the stack trace looks like after this change.

I can't find it either, but I chimed in on that too. Error[ERR_ASSERTION] looks like an array value. At an absolute minimum, please put a space in there: Error [ERR_ASSERTION]:

If @evanlucas's output is accurate, the { and } surrounding the output should also be removed. They suggest the contents are an object or JSON.

The { and } is an artifact of util.inspect() rendering, not the internal errors piece.... for instance, do

> var m = new Error("test")
undefined
> m.code = 1
1
> m
{ Error: test
    at repl:1:9
    at ContextifyScript.Script.runInThisContext (vm.js:44:33)
    at REPLServer.defaultEval (repl.js:239:29)
    at bound (domain.js:301:14)
    at REPLServer.runBound [as eval] (domain.js:314:12)
    at REPLServer.onLine (repl.js:433:10)
    at emitOne (events.js:120:20)
    at REPLServer.emit (events.js:210:7)
    at REPLServer.Interface._onLine (readline.js:262:10)
    at REPLServer.Interface._line (readline.js:611:8) code: 1 }

I will open a PR that adds the space before the opening [

I will open a PR that adds the space before the opening [

PR: #12099

Ping @nodejs/ctc ... I'd like to get this landed. Before the other errors ones get landed.

@jasnell This needs at least one more CTC approval before landing. Maybe you can horse-trade with @trevnorris: You review his async-hooks PR that he's not getting enough reviews on and in return he reviews this.

I'm kidding. Or am I?

I'd review the async-hooks had I the time. It's a large pr. I can take a look next week. There are quite a few other prs pending on this one, all semver major, that I would like to get landed

Maybe break out the expected.slice().join() into a variable? It is getting a little hard to read imo.

With only the first three arguments being used, maybe make this if (arguments.length === 3) {?

Feel free to disregard

@evanlucas ... do you still object to this?

@jasnell my review was not an objection. I think I'm onboard with this now.

Awesome, ok. Can I ask you to switch the "requested changes" to an approval or LGTM so I can get this landed. As a semver-major I need two CTC signoffs :-)

* Assign codes to the handful of errors reported by internal/process/*.js * Include documentation for the new error codes * Improve error messages * Improve test coverage for process.nextTick Ref: nodejs#11273

Final CI before landing: https://ci.nodejs.org/job/node-test-pull-request/7553/
CIGTM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/720/

CI failures are unrelated. Failures in CITGM are not specific to this PR.

* Assign codes to the handful of errors reported by internal/process/*.js * Include documentation for the new error codes * Improve error messages * Improve test coverage for process.nextTick PR-URL: #11294 Ref: #11273 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

Landed in e75bc87

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 the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Feb 10, 2017

jasnell mentioned this pull request Feb 10, 2017

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

Closed

80 tasks

mscdex added the process Issues and PRs related to the process subsystem. label Feb 10, 2017

joyeecheung reviewed Feb 11, 2017

View reviewed changes

no23reason mentioned this pull request Feb 13, 2017

errors, repl: migrate to use internal/errors.js #11347

Closed

4 tasks

jasnell force-pushed the internal-errors-1 branch from 18670e4 to e24a450 Compare February 14, 2017 15:46

jasnell requested review from addaleax, cjihrig, evanlucas and mhdawson February 14, 2017 15:50

bougarfaoui suggested changes Feb 14, 2017

View reviewed changes

jasnell force-pushed the internal-errors-1 branch from e24a450 to 1727832 Compare February 14, 2017 17:56

joyeecheung reviewed Feb 14, 2017

View reviewed changes

jasnell force-pushed the internal-errors-1 branch 3 times, most recently from 7f1def6 to 86c7993 Compare February 16, 2017 20:24

rauno56 mentioned this pull request Feb 26, 2017

WIP: module,errors: port module to internal/errors #11565

Closed

4 tasks

jasnell force-pushed the internal-errors-1 branch from 86c7993 to 9d3c44f Compare March 6, 2017 23:35

sousandrei mentioned this pull request Mar 8, 2017

dns, net, internal/net: migrate to internals/error #11738

Closed

4 tasks

Trott mentioned this pull request Apr 3, 2017

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

Closed

evanlucas suggested changes Apr 7, 2017

View reviewed changes

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

evanlucas approved these changes Apr 20, 2017

View reviewed changes

jasnell force-pushed the internal-errors-1 branch from 6ddc9a2 to 626332f Compare April 20, 2017 15:57

errors: port internal/process errors to internal/errors

8dbce73

* Assign codes to the handful of errors reported by internal/process/*.js * Include documentation for the new error codes * Improve error messages * Improve test coverage for process.nextTick Ref: nodejs#11273

jasnell force-pushed the internal-errors-1 branch from 626332f to 8dbce73 Compare April 20, 2017 15:58

jasnell closed this Apr 20, 2017

Trott removed the ctc-review label Apr 24, 2017

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

errors, process: port internal/process errors to internal/errors by jasnell · Pull Request #11294 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

errors, process: port internal/process errors to internal/errors#11294jasnell wants to merge 1 commit intonodejs:masternodejs/node:masterfrom jasnell:internal-errors-1jasnell/node:internal-errors-1Copy head branch name to clipboard

Conversation

jasnell commented Feb 10, 2017

Checklist

Affected core subsystem(s)

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 Feb 14, 2017

Uh oh!

jasnell commented Feb 14, 2017

Uh oh!

bougarfaoui left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Feb 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Feb 16, 2017

Uh oh!

jasnell commented Mar 6, 2017

Uh oh!

jasnell commented Mar 27, 2017

Uh oh!

evanlucas commented Mar 27, 2017

Uh oh!

jasnell commented Mar 27, 2017

Uh oh!

evanlucas commented Mar 27, 2017

Uh oh!

jasnell commented Mar 27, 2017

Uh oh!

Trott commented Mar 28, 2017

Uh oh!

jasnell commented Mar 28, 2017

Uh oh!

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

Uh oh!

Uh oh!

jasnell commented Apr 5, 2017

Uh oh!

Trott commented Apr 5, 2017

Uh oh!

jasnell commented Apr 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 19, 2017

Uh oh!

evanlucas commented Apr 19, 2017

Uh oh!

jasnell commented Apr 19, 2017

Uh oh!

jasnell commented Apr 20, 2017

Uh oh!

jasnell commented Apr 20, 2017

Uh oh!

jasnell commented Apr 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

errors, process: port internal/process errors to internal/errors#11294
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:internal-errors-1

gibfahn commented Mar 28, 2017 •
edited
Loading