errors: assign error code to bootstrap_node created error by jasnell · Pull Request #11298 · nodejs/node · GitHub

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

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

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?

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

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

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

LGTM with the naming sorted out

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

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.

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

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.

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

Updated. PTAL

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

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

LGTM

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

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".

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.

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

Nit: There is a colon in errors.js file

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

@Fishrock123 ... are you satisfied with this?

LGTM with a nit

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

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

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>

Landed in 251e5ed

Hmm, ok sounds good to me. Belated LGTM.

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 10, 2017

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 10, 2017

jasnell mentioned this pull request Feb 10, 2017

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

Closed

80 tasks

joyeecheung reviewed Feb 10, 2017

View reviewed changes

joyeecheung approved these changes Feb 11, 2017

View reviewed changes

thefourtheye reviewed Feb 12, 2017

View reviewed changes

joyeecheung reviewed Feb 14, 2017

View reviewed changes

jasnell force-pushed the internal-errors-2 branch from dcdda51 to 40637de Compare February 14, 2017 16:12

joyeecheung approved these changes Feb 14, 2017

View reviewed changes

joyeecheung mentioned this pull request Feb 14, 2017

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

Closed

4 tasks

jasnell added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Feb 14, 2017

jasnell force-pushed the internal-errors-2 branch from 40637de to 7a94c93 Compare March 22, 2017 21:29

jasnell added the ctc-review label Mar 22, 2017

targos approved these changes Mar 23, 2017

View reviewed changes

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 approved these changes 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-2 branch from 7a94c93 to 7707ed9 Compare April 20, 2017 19:05

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

joyeecheung approved these changes Apr 21, 2017

View reviewed changes

thefourtheye approved these changes Apr 21, 2017

View reviewed changes

jasnell force-pushed the internal-errors-2 branch from 7707ed9 to 491185c Compare April 21, 2017 17:04

Trott mentioned this pull request Apr 24, 2017

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

Closed

targos approved these changes Apr 24, 2017

View reviewed changes

jasnell force-pushed the internal-errors-2 branch from 491185c to de5c440 Compare April 24, 2017 16:05

jasnell closed this Apr 24, 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

errors: assign error code to bootstrap_node created error by jasnell · Pull Request #11298 · nodejs/node · GitHub

Uh oh!

Uh oh!

errors: assign error code to bootstrap_node created error - #11298

errors: assign error code to bootstrap_node created error#11298jasnell wants to merge 1 commit intonodejs:masternodejs/node:masterfrom jasnell:internal-errors-2jasnell/node:internal-errors-2Copy head branch name to clipboard

Conversation

jasnell commented Feb 10, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Fishrock123 commented Feb 10, 2017

Uh oh!

jasnell commented Feb 10, 2017

Uh oh!

joyeecheung Feb 10, 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!

jasnell Feb 10, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Feb 11, 2017

Uh oh!

joyeecheung commented Feb 11, 2017

Uh oh!

jasnell commented Feb 11, 2017

Uh oh!

joyeecheung commented Feb 12, 2017

Uh oh!

thefourtheye Feb 12, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell Feb 13, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung Feb 14, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Feb 14, 2017

Uh oh!

jasnell commented Mar 22, 2017

Uh oh!

jasnell commented Mar 22, 2017

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 20, 2017

Uh oh!

Fishrock123 commented Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Apr 20, 2017

Uh oh!

joyeecheung commented Apr 21, 2017

Uh oh!

thefourtheye Apr 21, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 21, 2017

Uh oh!

jasnell commented Apr 21, 2017

Uh oh!

targos left a comment

Choose a reason for hiding this comment

Uh oh!

targos Apr 24, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Apr 24, 2017

Uh oh!

Fishrock123 commented Apr 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

errors: assign error code to bootstrap_node created error#11298
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:internal-errors-2

joyeecheung Feb 10, 2017 •
edited
Loading

Fishrock123 commented Apr 20, 2017 •
edited
Loading