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: assign error code to bootstrap_node created error #11298

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@jasnell
Member

jasnell commented Feb 10, 2017

This does not use the internal/errors.js module because the error in question may actually be caused by an attempt to load internal/errors.js. This error should only be encountered in the case of a bug within Node.js itself.

Ref: #11273

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

errors, src

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Feb 10, 2017

Member

This is perhaps misleading as these are not necessarily "native" as in "C/C++" modules.

Member

Fishrock123 commented Feb 10, 2017

This is perhaps misleading as these are not necessarily "native" as in "C/C++" modules.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 10, 2017

Member

Yep. It certainly is. Basing this off the existing wording (I did not change the error message at all). Do you have a suggestion for an alternative?

Member

jasnell commented Feb 10, 2017

Yep. It certainly is. Basing this off the existing wording (I did not change the error message at all). Do you have a suggestion for an alternative?

Show outdated Hide outdated doc/api/errors.md
@@ -563,6 +563,17 @@ found [here][online].
encountered by [`http`][] or [`net`][] -- often a sign that a `socket.end()`
was not properly called.
<a id="nodejs-error-codes">
## Node.js Error Codes

This comment has been minimized.

@joyeecheung

joyeecheung Feb 10, 2017

Member

We probably need a section explaining the new error system(like the section on "system errors" above mentioning those would have error.code, error. errno and error. syscall properties, then list the error codes). Going straight to error code descriptions seems pretty confusing if people read it from the top to the bottom...

@joyeecheung

joyeecheung Feb 10, 2017

Member

We probably need a section explaining the new error system(like the section on "system errors" above mentioning those would have error.code, error. errno and error. syscall properties, then list the error codes). Going straight to error code descriptions seems pretty confusing if people read it from the top to the bottom...

This comment has been minimized.

@jasnell

jasnell Feb 10, 2017

Member

Yep, I agree. I was thinking of doing that in a separate PR

@jasnell

jasnell Feb 10, 2017

Member

Yep, I agree. I was thinking of doing that in a separate PR

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Feb 11, 2017

Member

@jasnell @Fishrock123 do we have a name for the lib modules in general?

Member

joyeecheung commented Feb 11, 2017

@jasnell @Fishrock123 do we have a name for the lib modules in general?

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Feb 11, 2017

Member

LGTM with the naming sorted out

Member

joyeecheung commented Feb 11, 2017

LGTM with the naming sorted out

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 11, 2017

Member

@joyeecheung ... not really with regards to naming. Perhaps built-in or internal would work?

Member

jasnell commented Feb 11, 2017

@joyeecheung ... not really with regards to naming. Perhaps built-in or internal would work?

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Feb 12, 2017

Member

Sorry, I wasn't being clear. I meant I am fine with native as long as we have a decision native is the word we use for lib modules, whichever we use we should stick to that :D

I do think built-in would be more unambiguous though. For contributors internal sounds like lib/internal modules, but if I understand correctly this covers unknown lib modules.

Member

joyeecheung commented Feb 12, 2017

Sorry, I wasn't being clear. I meant I am fine with native as long as we have a decision native is the word we use for lib modules, whichever we use we should stick to that :D

I do think built-in would be more unambiguous though. For contributors internal sounds like lib/internal modules, but if I understand correctly this covers unknown lib modules.

Show outdated Hide outdated doc/api/errors.md
@@ -563,6 +563,17 @@ found [here][online].
encountered by [`http`][] or [`net`][] -- often a sign that a `socket.end()`
was not properly called.
<a id="nodejs-error-codes">

This comment has been minimized.

@thefourtheye

thefourtheye Feb 12, 2017

Contributor

Why don't we let the tools to generate the id for all these? It would be consistent that way.

@thefourtheye

thefourtheye Feb 12, 2017

Contributor

Why don't we let the tools to generate the id for all these? It would be consistent that way.

This comment has been minimized.

@jasnell

jasnell Feb 13, 2017

Member

Because these anchors need to be consistent and predictable even if the structure of the document changes. Doing it this way ensures that the anchors will not change.

@jasnell

jasnell Feb 13, 2017

Member

Because these anchors need to be consistent and predictable even if the structure of the document changes. Doing it this way ensures that the anchors will not change.

Show outdated Hide outdated lib/internal/errors.js
@@ -85,4 +85,5 @@ module.exports = exports = {
//
// Note: Please try to keep these in alphabetical order
E('ERR_ASSERTION', (msg) => msg);
E('ERR_UNKNOWN_NATIVE_MODULE', (id) => `No such built-in module: ${id}`);

This comment has been minimized.

@joyeecheung

joyeecheung Feb 14, 2017

Member

Now that the error message is updated to No such built-in module, the error code should be changed accordingly?

@joyeecheung

joyeecheung Feb 14, 2017

Member

Now that the error message is updated to No such built-in module, the error code should be changed accordingly?

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 14, 2017

Member

Updated. PTAL

Member

jasnell commented Feb 14, 2017

Updated. PTAL

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 22, 2017

Member

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

Member

jasnell commented Mar 22, 2017

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

@jasnell

This comment has been minimized.

Show comment
Hide comment

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

@targos

targos approved these changes Mar 23, 2017

@mhdawson

LGTM

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 20, 2017

Member

This is no longer blocked. PR updated.
@targos @mhdawson @joyeecheung ... PTAL, does this still LGTY?

Member

jasnell commented Apr 20, 2017

This is no longer blocked. PR updated.
@targos @mhdawson @joyeecheung ... PTAL, does this still LGTY?

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Apr 20, 2017

Member

I think this is a misnomer, IIRC that module does not need to be built-in to possibly hit this path? (Don't all native modules hit this or something?)

Note: I understand it is called "NativeModule as in "builtin natives".

Member

Fishrock123 commented Apr 20, 2017

I think this is a misnomer, IIRC that module does not need to be built-in to possibly hit this path? (Don't all native modules hit this or something?)

Note: I understand it is called "NativeModule as in "builtin natives".

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 20, 2017

Member

Not that I'm aware of. In every condition that I was able to find, most users will only see the "Cannot find module" error. This appears to be hit only in very specific cases involving the built-in modules. I could be wrong, but that's what I've been able to see. There's aren't even any test cases that I can see that trigger/test this.

Member

jasnell commented Apr 20, 2017

Not that I'm aware of. In every condition that I was able to find, most users will only see the "Cannot find module" error. This appears to be hit only in very specific cases involving the built-in modules. I could be wrong, but that's what I've been able to see. There's aren't even any test cases that I can see that trigger/test this.

@joyeecheung

This comment has been minimized.

Show comment
Hide comment
@joyeecheung

joyeecheung Apr 21, 2017

Member

@jasnell FWIW, I have only ever hit this path when I accidentally deleted a js file from node.gyp...

Member

joyeecheung commented Apr 21, 2017

@jasnell FWIW, I have only ever hit this path when I accidentally deleted a js file from node.gyp...

Show outdated Hide outdated lib/internal/bootstrap_node.js
// Model the error off the internal/errors.js model, but
// do not use that module given that it could actually be
// the one causing the error if there's a bug in Node.js
const err = new Error(`No such built-in module ${id}`);

This comment has been minimized.

@thefourtheye

thefourtheye Apr 21, 2017

Contributor

Nit: There is a colon in errors.js file

@thefourtheye

thefourtheye Apr 21, 2017

Contributor

Nit: There is a colon in errors.js file

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 21, 2017

Member

@Fishrock123 ... are you satisfied with this?

Member

jasnell commented Apr 21, 2017

@Fishrock123 ... are you satisfied with this?

@targos

targos approved these changes Apr 24, 2017

LGTM with a nit

Show outdated Hide outdated lib/internal/bootstrap_node.js
// the one causing the error if there's a bug in Node.js
const err = new Error(`No such built-in module: ${id}`);
err.code = 'ERR_UNKNOWN_BUILTIN_MODULE';
err.name = 'Error[ERR_UNKNOWN_BUILTIN_MODULE]';

This comment has been minimized.

@targos

targos Apr 24, 2017

Member

nit: there should be a space now that 7b4a72d has landed.

@targos

targos Apr 24, 2017

Member

nit: there should be a space now that 7b4a72d has landed.

errors: assign error code to bootstrap_node created error
This does not use the internal/errors.js module because the error
in question may actually be *caused* by an attempt to load
internal/errors.js. This error should only be encountered in the
case of a bug within Node.js itself.

Ref: #11273

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

errors: assign error code to bootstrap_node created error
This does not use the internal/errors.js module because the error
in question may actually be *caused* by an attempt to load
internal/errors.js. This error should only be encountered in the
case of a bug within Node.js itself.

PR-URL: #11298
Ref: #11273
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 24, 2017

Member

Landed in 251e5ed

Member

jasnell commented Apr 24, 2017

Landed in 251e5ed

@jasnell jasnell closed this Apr 24, 2017

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Apr 24, 2017

Member

Hmm, ok sounds good to me. Belated LGTM.

Member

Fishrock123 commented Apr 24, 2017

Hmm, ok sounds good to me. Belated LGTM.

@Trott Trott removed the ctc-review label May 1, 2017

@jasnell jasnell referenced this pull request May 11, 2017

Closed

8.0.0 Release Proposal #12220

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