Node: Tracking Issue: Migrate errors to internal/errors.js

[jasnell]

Now that https://github.com/nodejs/node/pull/11220 has landed, we need to begin the process of migrating errors in the */lib.js source over to use it. A basic guide is provided here.

Note that moving existing errors over to this mechanism should, in general, be considered semver-major.

Please use the following list to track which files have been migrated over to using the new errors and provide a link back to this issue in the relevant PRs

• Internal
  
  
  
  • cluster (No changes required here)
  
  
  • [x] [child.js](https://github.com/nodejs/node/tree/master/lib/internal/cluster/child.js)
  
  
  • [x] [master.js](https://github.com/nodejs/node/tree/master/lib/internal/cluster/master.js)
  
  
  • [x] [round_robin_handle.js](https://github.com/nodejs/node/tree/master/lib/internal/cluster/round_robin_handle.js)
  
  
  • [x] [shared_handle.js](https://github.com/nodejs/node/tree/master/lib/internal/cluster/shared_handle.js)
  
  
  • [x] [utils.js](https://github.com/nodejs/node/tree/master/lib/internal/cluster/utils.js)
  
  
  • [x] [worker.js](https://github.com/nodejs/node/tree/master/lib/internal/cluster/worker.js)
  
  
  • process
  
  
  • [x] [next_tick.js](https://github.com/nodejs/node/tree/master/lib/internal/process/next_tick.js) - https://github.com/nodejs/node/pull/13982
  
  
  • [x] [promises.js](https://github.com/nodejs/node/tree/master/lib/internal/process/promises.js) - https://github.com/nodejs/node/pull/11294
  
  
  • [x] [stdio.js](https://github.com/nodejs/node/tree/master/lib/internal/process/stdio.js) - https://github.com/nodejs/node/pull/11294
  
  
  • [x] [warning.js](https://github.com/nodejs/node/tree/master/lib/internal/process/warning.js) - https://github.com/nodejs/node/pull/11294
  
  
  • streams (No changes required here)
  
  
  • [x] [BufferList.js](https://github.com/nodejs/node/tree/master/lib/internal/streams/BufferList.js)
  
  
  • [x] [lazy_transform.js](https://github.com/nodejs/node/tree/master/lib/internal/streams/lazy_transform.js)
  
  
  • [x] [legacy.js](https://github.com/nodejs/node/tree/master/lib/internal/streams/legacy.js)
  
  
  • [x] [bootstrap_node.js](https://github.com/nodejs/node/tree/master/lib/internal/bootstrap_node.js) - https://github.com/nodejs/node/pull/11298
  
  
  • [x] [buffer.js](https://github.com/nodejs/node/tree/master/lib/internal/buffer.js) - https://github.com/nodejs/node/pull/11341
  
  
  • [X] [child_process.js](https://github.com/nodejs/node/tree/master/lib/internal/child_process.js) - https://github.com/nodejs/node/pull/11300 + https://github.com/nodejs/node/pull/14009
  
  
  • [x] [errors.js](https://github.com/nodejs/node/tree/master/lib/internal/errors.js) (No need to migrate itself)
  
  
  • [x] [freelist.js](https://github.com/nodejs/node/tree/master/lib/internal/freelist.js) (No changes required here)
  
  
  • [X] [fs.js](https://github.com/nodejs/node/tree/master/lib/internal/fs.js) - https://github.com/nodejs/node/pull/11317, https://github.com/nodejs/node/pull/13829,
  
  
  • [x] [linkedlist.js](https://github.com/nodejs/node/tree/master/lib/internal/linkedlist.js) (No changes required here)
  
  
  • [x] [module.js](https://github.com/nodejs/node/tree/master/lib/internal/module.js) (No changes required here)
  
  
  • [x] [net.js](https://github.com/nodejs/node/tree/master/lib/internal/net.js) - https://github.com/nodejs/node/pull/11302
  
  
  • [x] [process.js](https://github.com/nodejs/node/tree/master/lib/internal/process.js)
  
  
  • [x] [readline.js](https://github.com/nodejs/node/tree/master/lib/internal/readline.js)
  
  
  • [x] [repl.js](https://github.com/nodejs/node/tree/master/lib/internal/repl.js)
  
  
  • [x] [socket_list.js](https://github.com/nodejs/node/tree/master/lib/internal/socket_list.js) - https://github.com/nodejs/node/pull/11356
  
  
  • [x] [url.js](https://github.com/nodejs/node/tree/master/lib/internal/url.js) - https://github.com/nodejs/node/pull/12574
  
  
  • [x] [util.js](https://github.com/nodejs/node/tree/master/lib/internal/util.js) - https://github.com/nodejs/node/pull/11301, https://github.com/nodejs/node/pull/13829
  
  
  • [x] [v8_prof_polyfill.js](https://github.com/nodejs/node/tree/master/lib/internal/v8_prof_polyfill.js) (No changes required here)
  
  
  • [x] [v8_prof_processor.js](https://github.com/nodejs/node/tree/master/lib/internal/v8_prof_processor.js) (No changes required here)
  
  
• _debug_agent.js (removed in https://github.com/nodejs/node/commit/549e81bfa1)
• _debugger.js - https://github.com/nodejs/node/pull/11380 (File removed from master)
• [x] [_http_agent.js](https://github.com/nodejs/node/tree/master/lib/_http_agent.js)
• [x] [_http_client.js](https://github.com/nodejs/node/tree/master/lib/_http_client.js) - https://github.com/nodejs/node/pull/14423
• [x] [_http_common.js](https://github.com/nodejs/node/tree/master/lib/_http_common.js)
• [x] [_http_incoming.js](https://github.com/nodejs/node/tree/master/lib/_http_incoming.js)
• [x] [_http_outgoing.js](https://github.com/nodejs/node/tree/master/lib/_http_outgoing.js) https://github.com/nodejs/node/pull/14735
• [x] [_http_server.js](https://github.com/nodejs/node/tree/master/lib/_http_server.js)
• ~_linklist.js~
• [x] [_tls_common.js](https://github.com/nodejs/node/tree/master/lib/_tls_common.js) (No changes needed here)
• [x] [_tls_legacy.js](https://github.com/nodejs/node/tree/master/lib/_tls_legacy.js) (No changes needed here)
• [x] [_tls_wrap.js](https://github.com/nodejs/node/tree/master/lib/_tls_wrap.js) - https://github.com/nodejs/node/pull/13476
• [x] [assert.js](https://github.com/nodejs/node/tree/master/lib/assert.js)
• [x] [async_hooks.js](https://github.com/nodejs/node/blob/master/lib/async_hooks.js) https://github.com/nodejs/node/pull/14722/
• [x] [buffer.js](https://github.com/nodejs/node/tree/master/lib/buffer.js) #13976
• [x] [child_process.js](https://github.com/nodejs/node/tree/master/lib/child_process.js) https://github.com/nodejs/node/pull/15090
• [x] [cluster.js](https://github.com/nodejs/node/tree/master/lib/cluster.js) (No changes required here)
• [x] [console.js](https://github.com/nodejs/node/tree/master/lib/console.js) - https://github.com/nodejs/node/pull/11308
• [x] [constants.js](https://github.com/nodejs/node/tree/master/lib/constants.js) (No changes required here)
• [x] [crypto.js](https://github.com/nodejs/node/tree/master/lib/crypto.js)
• [x] [dgram.js](https://github.com/nodejs/node/tree/master/lib/dgram.js) - #12926
• [x] [dns.js](https://github.com/nodejs/node/tree/master/lib/dns.js) - https://github.com/nodejs/node/pull/14212
• [x] [domain.js](https://github.com/nodejs/node/tree/master/lib/domain.js)
• [x] [events.js](https://github.com/nodejs/node/tree/master/lib/events.js) - https://github.com/nodejs/node/pull/15623
• [x] [fs.js](https://github.com/nodejs/node/tree/master/lib/fs.js) - https://github.com/nodejs/node/pull/15043
• [x] [http.js](https://github.com/nodejs/node/tree/master/lib/http.js) (No changes required here)
• [x] [https.js](https://github.com/nodejs/node/tree/master/lib/https.js) - https://github.com/nodejs/node/pull/15603
• [x] [inspector.js](https://github.com/nodejs/node/tree/master/lib/inspector.js) https://github.com/nodejs/node/pull/15619
• [x] [module.js](https://github.com/nodejs/node/tree/master/lib/module.js)
• [x] [net.js](https://github.com/nodejs/node/tree/master/lib/net.js) - https://github.com/nodejs/node/pull/14782
• [x] [os.js](https://github.com/nodejs/node/tree/master/lib/os.js) (No changes required here)
• [x] [path.js](https://github.com/nodejs/node/tree/master/lib/path.js) - https://github.com/nodejs/node/pull/11319
• [x] [process.js](https://github.com/nodejs/node/tree/master/lib/process.js) (No changes required here)
• [ ] [punycode.js](https://github.com/nodejs/node/tree/master/lib/punycode.js) (may not need to do this one)
• [x] [querystring.js](https://github.com/nodejs/node/tree/master/lib/querystring.js) - https://github.com/nodejs/node/pull/15565
• [x] [readline.js](https://github.com/nodejs/node/tree/master/lib/readline.js) - https://github.com/nodejs/node/pull/11390
• [x] [repl.js](https://github.com/nodejs/node/tree/master/lib/repl.js) - https://github.com/nodejs/node/pull/11347
• [x] [timers.js](https://github.com/nodejs/node/tree/master/lib/timers.js) - https://github.com/nodejs/node/pull/14659
• [x] [tls.js](https://github.com/nodejs/node/tree/master/lib/tls.js) - https://github.com/nodejs/node/pull/13994
• [x] [tty.js](https://github.com/nodejs/node/tree/master/lib/tty.js)
• [x] [url.js](https://github.com/nodejs/node/tree/master/lib/url.js) - https://github.com/nodejs/node/pull/13963
• [x] [util.js](https://github.com/nodejs/node/tree/master/lib/util.js)
• [x] [v8.js](https://github.com/nodejs/node/tree/master/lib/v8.js) (no changes required here)
• [x] [vm.js](https://github.com/nodejs/node/tree/master/lib/vm.js)
• [x] [zlib.js](https://github.com/nodejs/node/tree/master/lib/zlib.js) - https://github.com/nodejs/node/pull/15618

stream related (blocked)

• [x] [_stream_duplex.js](https://github.com/nodejs/node/tree/master/lib/_stream_duplex.js)
• [x] [_stream_passthrough.js](https://github.com/nodejs/node/tree/master/lib/_stream_passthrough.js)
• [x] [_stream_readable.js](https://github.com/nodejs/node/tree/master/lib/_stream_readable.js)
• [x] [_stream_transform.js](https://github.com/nodejs/node/tree/master/lib/_stream_transform.js)
• [x] [_stream_wrap.js](https://github.com/nodejs/node/tree/master/lib/_stream_wrap.js) - https://github.com/nodejs/node/pull/13291
• [x] [_stream_writable.js](https://github.com/nodejs/node/tree/master/lib/_stream_writable.js)
• [x] [string_decoder.js](https://github.com/nodejs/node/tree/master/lib/string_decoder.js) - https://github.com/nodejs/node/pull/14682
• [x] [stream.js](https://github.com/nodejs/node/tree/master/lib/stream.js)

_{@refack: removed GFC label and commented out sentence in description + split off stream stuff
@BridgeAR: updated the list}

[fl0w]

Am I wrong in thinking this could be a "good first contribution"? Would love to give it a try in that case.
Also, I'm assuming */lib.js is a typo for lib/*.js?

[joyeecheung]

I think this could be a good first contribution in general, though some of the errors could be more complicated(the ones that are more frequently getting parsed in userland).

Also, I'm assuming /lib.js is a typo for lib/.js?

Probably lib/**/*.js :)

[joyeecheung]

@jasnell I've added links to those files, hope I am doing this right..

[jasnell]

@fl0w ... yes, this can be a good first contribution. Please use my PR https://github.com/nodejs/node/pull/11294 as a model. New error codes are added to internal/errors.js. Please reuse existing codes as possible. For instance, my PR #11294 introduces the ERR_INVALID_ARG_TYPE error code. If you're going through and making changes before #11294 lands, and you need ERR_INVALID_ARG_TYPE, duplicate it from #11294 as a separate commit in your PR. Then, if #11294 lands first, you can rebase and drop that commit, but if your PR lands first, then I can rebase and drop it from mine, etc. (hopefully that makes sense).

Also please make sure that descriptions for the error codes are added to docs/api/errors.md the way I've illustrated in #11294.

Really appreciate your willingness to jump in! Let me know if you have any questions or issues!

[seppevs]

@jasnell: can we modify the error messages of existing errors to more generic and reusable error messages? Or should the error message be completely backwards compatible?

[jasnell]

Modifying the error message is certainly possible. The biggest thing is to avoid duplicating error codes so make sure you check the other PRs for codes that may be reusable.

[joyeecheung]

Do we need to launch CITGM for all these semver-major PRs? cc @nodejs/build

[jasnell]

We should, yes.

[gbrmachado]

Hello @jasnell .
I'd like to give a try on it. Should I pick any item of this list without PR and create a PR by myself or should you create the PR?

Thanks

[joyeecheung]

@gbrmachado I think the idea is you should open a PR by yourself if you've picked an item from the list that has not been PR'ed by other people :D

[gunar]

Here's a PR for internal/fs.js: #11317
Please add to the list. Thanks!

[joyeecheung]

@gunar I've added 11317 to the original post

Also when adding some more PRs I discovered https://github.com/nodejs/node/pull/11308 and https://github.com/nodejs/node/pull/11340 are duplicates. I've picked 11308 because it is earlier.

[joyeecheung]

Also, for those who want to submit a PR for this as their first contribution, you can check out https://github.com/nodejs/node/pulls?q=is%3Aopen+is%3Apr+label%3Aerrors in addition to check out the original post because manual updates might lag behind a little. Sorry for the inconvenience, really don't want to see a PR gets closed because it's a duplicate >__<

[joyeecheung]

I've added more PRs to the OP and checked some internal modules as well as os.js and constants.js as "No changes required here" because they don't construct new errors at the moment. Let me know if I've made a mistake.

Also does this mean that we should ask PRs that add new/modify errors (but don't intend to do the migration) to use the new error system instead?

[jasnell]

Also does this mean that we should ask PRs that add new/modified errors ...

Not necessarily just yet. I've been intentionally holding off on landing these because I want to make sure @nodejs/ctc is good with the way things are going.

[sergio-rojasa]

I would like to work on this. I never contributed before but this seems like a good starting point.

[sergio-rojasa]

@jasnell can you mentor me.

[jasnell]

Yes absolutely. I will be unavailable for most of Wednesday but can definitely help later in the week

[zak-grumbles]

I've mostly finished migrating util.js, but I'm running into an issue when running test-util-format.js. Who should I get in contact with about this?

Thanks in advance for the help!

[Trott]

@zak-grumbles If you are an IRC user, you can ask for help in the #node-dev channel on Freenode IRC.

[Rauno56]

I'll take a stab at porting module.js in #11565.

[tjhosford]

Is this still taking contributions? I'm interested in taking a stab at some but noticed there are a lot of open PRs, which presumably could make it difficult to resolve conflicts...

[joyeecheung]

@TomHoss I think these PRs are waiting for CITGM(still kinda broken) and more CTC reviews? @jasnell Has this issue gone to ctc-agenda before?

[JeroenMandersloot]

I'm posting here on behalf of a small group of 4 developers who are looking to make a first contribution to Node! This seemed like the best place to start, so if possible we would greatly appreciate it if @jasnell or someone else would be available to mentor us.

[luidynaves]

@jasnell I've worked with zlib.js migration errors, but it's my first contribution so far. Who are the people who open the respective issue?

[BridgeAR]

The following should be edited:

// ... I didn't check the ones above

• [x] _stream_wrap.js
• [ ] _stream_writable.js
• [x] _tls_common.js (No changes required here)
• [x] _tls_legacy.js (No changes required here)
• [ ] _tls_wrap.js
• [x] assert.js
• [ ] async_hooks.js
• [ ] buffer.js
• [ ] child_process.js
• [x] cluster.js (No changes required here)
• [x] console.js
• [x] constants.js (No changes required here)
• [ ] crypto.js
• [x] dgram.js
• [ ] dns.js
• [x] domain.js (No changes required here)
• [ ] events.js - #11400
• [ ] fs.js
• [x] http.js (No changes required here)
• [ ] https.js - #11293
• [ ] module.js
• [ ] net.js
• [x] os.js (No changes required here)
• [x] path.js
• [x] process.js (No changes required here)
• [ ] punycode.js (may not need to do this one)
• [ ] querystring.js - #11398
• [x] readline.js
• [x] repl.js
• [x] stream.js (No changes required here)
• [ ] string_decoder.js - #11385
• [ ] timers.js - #11384
• [x] tls.js (No changes required here)
• [x] tty.js
• [ ] url.js - #11360
• [x] util.js
• [ ] v8.js
• [x] vm.js
• [ ] zlib.js

Some intercept errors and I am not sure if they should be touched or not but I think as soon as all regular errors are already modified to be internal errors, the interception should not be touched. E.g. domains do that.

[mhdawson]

I think I've updated the main description based on updates posted as comments. In the future it would probably be easier if just the lines that were changed were posted.

[zladuric]

Hey @jasnell if I were to take on one of these (specifically fs.js), what would be the steps? Looking at some PRs here, I would:

• checkout a new branch (fs-internal-errors?)
• make changes to fs.js and internal/errors.js (egrep "new (Range|Type)?Error" ../node/lib/fs.js - 27 errors to replace`)
• change all the expectError assertions in test/parallel/test-fs-*.js (6 occurences in 94 files on a rough count)?

Edit: also how would I handle that backtrace error in rethrow()?

And than the rest from CONTRIBUTING.md, rebase and run tests.

Edit 2: Initial stab at this is here: https://github.com/zladuric/node/commit/c07d2fceb6bffb8c6b8cda4ae8aad917f3593b83

More questions:

• can you check and see if I'm doing the test changes right?
• do I also need to change all assert.throws? in test-fs-*.js? How would I do those?
• specifically,
• anything else except those two (common.expectsError and assert.throws)?
• do I add all the errors in doc/api/errors.md? How to sort them/include them?

Thanks for the help in me helping node. This is actually first contribution so I'm kind of awed :)

[refack]

@mcollina any other modules should be blocked until we have a modular solution for readable_stream?

[mcollina]

@refack no others.

[Mirodil]

Hi,
Might be it is out of scope, just was reading a basic guide and noticed, I think, the error codes should be saved in constant variable then use them in other files.

[tniessen]

@Mirodil What do you mean? Something like this?

module.exports.constants = {
  ERR_INVALID_ARG_TYPE: 'ERR_INVALID_ARG_TYPE',
  ...
}

Which advantage would this offer?

[Mirodil]

@tniessen yes something like you showed, at the moment the error code name needs to be specify more then 2 places in string form(first place internal/errors.js, and in other files). What I thought

const ERR_INVALID_ARG_TYPE = 'ERR_INVALID_ARG_TYPE';
// in internal/errors.js
E(ERR_INVALID_ARG_TYPE, 'Expected string received %s');

// in file somename.js
const errors = require('internal/errors');
// ...
const err = new errors.TypeError(ERR_INVALID_ARG_TYPE, type);

// in file somename2.js
const errors = require('internal/errors');
// ...
const err = new errors.TypeError(ERR_INVALID_ARG_TYPE, type);

it gives one place for all error codes and prevents typos. might be not significant.

[tniessen]

@Mirodil Typos within node itself are unprobable as we usually test all error codes, but userland code cannot be protected from typos entirely. For example, if people use something like const ERR_INVALID_ARG_TYPE = require('node_errors').ERR_INVALID_ARG_TYPe (note the lowercase e), the value will be undefined and error.code === ERR_INVALID_ARG_TYPE will fail just as if they had a typo in the string. It is a good idea, but I don't think the advantages outweigh the associated maintenance.

[Mirodil]

totally agree

[styfle]

@tniessen True but TypeScript could catch this type of misspelling if the enum was exposed. Additionally, one could open the REPL and print all enum values if you're not using TypeScript and see what are the available choices.

Not that my opinion matters, but I like the idea of exposing the constants 😃

[pmatzavin]

lib/net.js convert errors over internal/errors.js PR #14782
Please add to the list. Thanks!

[refack]

Hi,
Might be it is out of scope, just was reading a basic guide and noticed, I think, the error codes should be saved in constant variable then use them in other files.

There is a longer term goal of modularizing the error codes into a packaged modules so that other packages could use the same codes. (It could also use a Proxy that throws instead of returning undefined).
_For the time being_ the new Errors are "internal" and are not ments to be used by 3rd party modules.

[pmatzavin]

Here's a PR for lib/fs.js: #15043
Please add to the list. Thanks!

[hiroppy]

Thanks, I added.

[ramimoshe]

Here's a PR for lib/child_process.js: #15090
Please add to the list. Thanks!

[BridgeAR]

I just updated the list in the original post to reflect our current state. A couple of entries that were unchecked are now checked and a few that originally had PRs do not have PRs and still have to be ported.

[ramimoshe]

I am on "querystring.js"

[ramimoshe]

Here's a PR for lib/querystring.js: #15565
Please add to the list. Thanks!

[BridgeAR]

I am closing this as outdated. All regular JS errors got migrated but might need some more polishing here and there. There are a couple c++ errors that should be migrated but this was never tracked here.

[joyeecheung]

@BridgeAR There seems to be a few new old-style errors added in JS since this PR opened, searching new Error\(.+\) yields 23 results in the current master. We should probably open a new issue for that.

[joyeecheung]

I believe this issue is mostly done now except the punycode ones. I opened https://github.com/nodejs/node/issues/27023 for that, so I am going to close this. Feel free to reopen if anyone thinks otherwise.

[joyeecheung]

Oops, did not realize it was closed. Sorry about the noise.