New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into
base: master
from

Conversation

@jasnell
Member

jasnell commented Feb 10, 2017

  • 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: #11273

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

@jasnell jasnell referenced this pull request Feb 10, 2017

Closed

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

78 of 80 tasks complete

@mscdex mscdex added the process label Feb 10, 2017

<a id="ERR_INVALID_ARG_TYPE"></a>
### ERR_INVALID_ARG_TYPE
The `'ERR_INVALID_ARG_TYPE'` error code is used generically to identify that

This comment has been minimized.

@joyeecheung

joyeecheung Feb 11, 2017

Member

(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)
@joyeecheung

joyeecheung Feb 11, 2017

Member

(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)

This comment has been minimized.

@joyeecheung

joyeecheung Feb 11, 2017

Member

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.

@joyeecheung

joyeecheung Feb 11, 2017

Member

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 comment has been minimized.

@jasnell

jasnell Feb 14, 2017

Member

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.

@jasnell

jasnell Feb 14, 2017

Member

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.

This comment has been minimized.

@joyeecheung

joyeecheung Feb 14, 2017

Member

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?

@joyeecheung

joyeecheung Feb 14, 2017

Member

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?

Show outdated Hide outdated lib/internal/errors.js
msg += `one of type ${expected.slice(0, len - 1).join(', ')}, or ` +
expected[len - 1];
} else {
msg += `type ${String(expected[0])}`;

This comment has been minimized.

@joyeecheung

joyeecheung Feb 11, 2017

Member

String(..) seems unnecessary here?

@joyeecheung

joyeecheung Feb 11, 2017

Member

String(..) seems unnecessary here?

This comment has been minimized.

@targos

targos Feb 11, 2017

Member

+1. It's already cast on line 103

@targos

targos Feb 11, 2017

Member

+1. It's already cast on line 103

@no23reason no23reason referenced this pull request Feb 13, 2017

Closed

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

4 of 4 tasks complete
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 14, 2017

Member

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

Member

jasnell commented Feb 14, 2017

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

@jasnell

This comment has been minimized.

Show comment
Hide comment

@jasnell jasnell requested review from evanlucas, addaleax, cjihrig and mhdawson Feb 14, 2017

@bougarfaoui

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

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 14, 2017

Member

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

Member

jasnell commented Feb 14, 2017

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

Show outdated Hide outdated lib/internal/errors.js
E('ERR_INVALID_CALLBACK', 'callback must be a function');
E('ERR_STDERR_CLOSE', 'process.stderr cannot be closed');
E('ERR_STDOUT_CLOSE', 'process.stdout cannot be closed');
E('ERR_UNK_STDIN_TYPE', 'Unknown stdin file type');

This comment has been minimized.

@joyeecheung

joyeecheung Feb 14, 2017

Member

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

@joyeecheung

joyeecheung Feb 14, 2017

Member

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

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 16, 2017

Member

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

Member

jasnell commented Feb 16, 2017

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

@Rauno56 Rauno56 referenced this pull request Feb 26, 2017

Closed

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

1 of 4 tasks complete
@jasnell

This comment has been minimized.

Show comment
Hide comment
Member

jasnell commented Mar 6, 2017

@sousandrei sousandrei referenced this pull request Mar 8, 2017

Closed

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

4 of 4 tasks complete
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 22, 2017

Member

@nodejs/ctc ... can I get some review and sign off on this?

Member

jasnell commented Mar 22, 2017

@nodejs/ctc ... can I get some review and sign off on this?

@jasnell jasnell added the ctc-review label Mar 22, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment

@targos targos referenced this pull request Mar 23, 2017

Closed

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

4 of 4 tasks complete
// Add new errors from here...
function invalidArgType(name, expected, actual) {

This comment has been minimized.

@targos

targos Mar 23, 2017

Member

Copying my comment from #11300:

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

@targos

targos Mar 23, 2017

Member

Copying my comment from #11300:

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

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 23, 2017

Member

@targos ... error message updated per the request. Note that #11300 will be updated after this lands to remove it's copy of the invalidArgType() method.

Member

jasnell commented Mar 23, 2017

@targos ... error message updated per the request. Note that #11300 will be updated after this lands to remove it's copy of the invalidArgType() method.

@targos

targos approved these changes Mar 24, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 24, 2017

Member

ping @nodejs/ctc ... I'd like to get this finished up. PTAL

Member

jasnell commented Mar 24, 2017

ping @nodejs/ctc ... I'd like to get this finished up. PTAL

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 27, 2017

Member

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

Member

jasnell commented Mar 27, 2017

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

@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Mar 27, 2017

Member

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.

Member

evanlucas commented Mar 27, 2017

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.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 27, 2017

Member

Which part are you unhappy with?

Member

jasnell commented Mar 27, 2017

Which part are you unhappy with?

@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Mar 27, 2017

Member
$ ./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:

Member

evanlucas commented Mar 27, 2017

$ ./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:

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 27, 2017

Member

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.

Member

jasnell commented Mar 27, 2017

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.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Mar 28, 2017

Member

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.

Member

Trott commented Mar 28, 2017

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.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 28, 2017

Member

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 [

Member

jasnell commented Mar 28, 2017

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 [

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Mar 28, 2017

Member

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

PR: #12099

Member

gibfahn commented Mar 28, 2017

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

PR: #12099

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 5, 2017

Member

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

Member

jasnell commented Apr 5, 2017

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

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 5, 2017

Member

@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?

Member

Trott commented Apr 5, 2017

@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?

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 5, 2017

Member

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

Member

jasnell commented Apr 5, 2017

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

var len = expected.length;
expected = expected.map((i) => String(i));
if (len > 1) {
msg += `one of type ${expected.slice(0, len - 1).join(', ')}, or ` +

This comment has been minimized.

@evanlucas

evanlucas Apr 7, 2017

Member

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

@evanlucas

evanlucas Apr 7, 2017

Member

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

} else {
msg += `of type ${String(expected)}`;
}
if (arguments.length >= 3) {

This comment has been minimized.

@evanlucas

evanlucas Apr 7, 2017

Member

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

Feel free to disregard

@evanlucas

evanlucas Apr 7, 2017

Member

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

Feel free to disregard

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 19, 2017

Member

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

Member

jasnell commented Apr 19, 2017

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

@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Apr 19, 2017

Member

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

Member

evanlucas commented Apr 19, 2017

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

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 19, 2017

Member

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 :-)

Member

jasnell commented Apr 19, 2017

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 :-)

errors: port internal/process errors to internal/errors
* 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: #11273
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 20, 2017

Member

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

Member

jasnell commented Apr 20, 2017

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

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

errors: port internal/process errors to internal/errors
* 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>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 20, 2017

Member

Landed in e75bc87

Member

jasnell commented Apr 20, 2017

Landed in e75bc87

@jasnell jasnell closed this Apr 20, 2017

@Trott Trott removed the ctc-review label Apr 24, 2017

@jasnell jasnell referenced this pull request May 11, 2017

Closed

8.0.0 Release Proposal #12220

@refack refack added this to Done in Error Codes Aug 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment