Skip to content

/node

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

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

Closed
jasnell opened this issue Feb 9, 2017 · 48 comments

Comments

Assignees
No one assigned
Labels
errors mentor-available meta
Projects
  
Done in Error Codes
Milestone
No milestone
@jasnell
Copy link
Member

commented Feb 9, 2017 •
edited by joyeecheung

Now that #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

stream related (blocked)

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

@mscdex mscdex added the errors label Feb 9, 2017

@fl0w

This comment has been minimized.

Sign in to view
Copy link

commented Feb 10, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Member

commented Feb 10, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Member

commented Feb 10, 2017

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

@seppevs seppevs referenced this issue Feb 10, 2017

Closed

https: use internal/errors.js #11293

2 of 2 tasks complete

@jasnell jasnell referenced this issue Feb 10, 2017

Closed

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

4 of 4 tasks complete
@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Feb 10, 2017

@fl0w ... yes, this can be a good first contribution. Please use my PR #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!

@jasnell jasnell referenced this issue Feb 10, 2017

Closed

errors: assign error code to bootstrap_node created error #11298

3 of 3 tasks complete

@jasnell jasnell added the good first issue label Feb 10, 2017

@joyeecheung joyeecheung referenced this issue Feb 10, 2017

Closed

New error system in WHATWG URL implementation #11299

@seppevs

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Feb 10, 2017

@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

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Feb 10, 2017

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.

@jasnell jasnell referenced this issue Feb 10, 2017

Closed

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

4 of 4 tasks complete

This was referenced Feb 10, 2017

Closed
Closed

slammayjammay added a commit to slammayjammay/node that referenced this issue Feb 11, 2017

@slammayjammay
errors, console: migrate to use internal/errors.js 
Refs: nodejs#11273
af32dc2

@slammayjammay slammayjammay referenced this issue Feb 11, 2017

Closed

errors, console: migrate to use internal/errors.js #11308

4 of 4 tasks complete
@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member

commented Feb 11, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Feb 11, 2017

We should, yes.

@gunar gunar referenced this issue Feb 12, 2017

Merged

errors: port internal/fs errors to internal/errors #11317

0 of 4 tasks complete

@seppevs seppevs referenced this issue Feb 12, 2017

Closed

errors, path: migrate to use internal/errors.js #11319

4 of 4 tasks complete

This was referenced Feb 13, 2017

Closed
Closed

@mskec mskec referenced this issue Feb 13, 2017

Closed

errors: migrate lib/console #11340

3 of 4 tasks complete

@bougarfaoui bougarfaoui referenced this issue Feb 13, 2017

Closed

errors: port internal/buffer errors to internal/errors #11341

0 of 4 tasks complete

@no23reason no23reason referenced this issue Feb 13, 2017

Closed

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

4 of 4 tasks complete

addaleax added a commit to addaleax/ayo that referenced this issue Sep 17, 2017

@starkwang @addaleax
string_decoder: Migrate to use internal/errors 
PR-URL: nodejs/node#14682
Refs: nodejs/node#11273
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
a08bf62

@kreig303 kreig303 referenced this issue Sep 18, 2017

Closed

errors crypto : migrate errors to internal/errors.js #15452

3 of 4 tasks complete

pmatzavin added a commit to pmatzavin/node that referenced this issue Sep 19, 2017

@pmatzavin
lib/net: Convert to using internal/errors 
Covert lib/net.js over to using lib/internal/errors.js

Ref: nodejs#11273

I have not addressed the cases that use errnoException(),
for reasons described in nodejsGH-12926

- Replace thrown errors in lib/net.js
  with errors from lib/internal/errors.
  The ERR_INVALID_OPT_VALUE error have been used
  in the Server.prototype.listen() method
  after a discussion in Ref: nodejs#14782
- Update tests according to the above modifications
ce932b1

Qard pushed a commit to Qard/ayo that referenced this issue Sep 21, 2017

@starkwang Stephen Belanger
string_decoder: Migrate to use internal/errors 
PR-URL: nodejs/node#14682
Refs: nodejs/node#11273
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
c62ac2f
@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member

commented Sep 23, 2017

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

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Sep 23, 2017

I am on "querystring.js"

@ramimoshe ramimoshe referenced this issue Sep 23, 2017

Merged

lib/querystring: convert to using internal/errors #15565

@ramimoshe

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Sep 23, 2017

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

@ramimoshe ramimoshe referenced this issue Sep 25, 2017

Closed

lib/https: convert to using internal/errors #15603

jasnell added a commit to pmatzavin/node that referenced this issue Sep 25, 2017

@pmatzavin @jasnell
lib/net: Convert to using internal/errors 
Covert lib/net.js over to using lib/internal/errors.js

Ref: nodejs#11273

I have not addressed the cases that use errnoException(),
for reasons described in nodejsGH-12926

- Replace thrown errors in lib/net.js
  with errors from lib/internal/errors.
  The ERR_INVALID_OPT_VALUE error have been used
  in the Server.prototype.listen() method
  after a discussion in Ref: nodejs#14782
- Update tests according to the above modifications
0cc1ea6

joyeecheung added a commit that referenced this issue Oct 15, 2017

@pmatzavin @joyeecheung
net: convert to using internal/errors 
Covert lib/net.js over to using lib/internal/errors.js

- Replace thrown errors in lib/net.js
  with errors from lib/internal/errors.
  The ERR_INVALID_OPT_VALUE error have been used
  in the Server.prototype.listen() method
- Update tests according to the above modifications

PR-URL: #14782
Refs: #11273
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
7f55349

addaleax added a commit to ayojs/ayo that referenced this issue Oct 15, 2017

@pmatzavin @addaleax
net: convert to using internal/errors 
Covert lib/net.js over to using lib/internal/errors.js

- Replace thrown errors in lib/net.js
  with errors from lib/internal/errors.
  The ERR_INVALID_OPT_VALUE error have been used
  in the Server.prototype.listen() method
- Update tests according to the above modifications

PR-URL: nodejs/node#14782
Refs: nodejs/node#11273
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
01a7b1c

starkwang added a commit to starkwang/node that referenced this issue Oct 21, 2017

@starkwang
buffer: convert buffer.transcode to use internal/errors 
`buffer.transcode` is still using raw TypeError. This change is to
convert it to use internal/errors.

Ref: nodejs#11273
da00b76

@starkwang starkwang referenced this issue Oct 21, 2017

Closed

buffer: convert `buffer.transcode` to use internal/errors #16352

3 of 3 tasks complete

starkwang added a commit to starkwang/node that referenced this issue Oct 22, 2017

@starkwang
buffer: convert buffer.transcode to use internal/errors 
`buffer.transcode` is still using raw TypeError. This change is to
convert it to use internal/errors.

Ref: nodejs#11273
0117db7

gireeshpunathil added a commit that referenced this issue Oct 23, 2017

@starkwang @gireeshpunathil
buffer: buffer.transcode to use internal/errors 
`buffer.transcode` is still using raw TypeError. This change is to
convert it to use internal/errors.

Ref: #11273
PR-URL: #16352
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
e79a61c

addaleax added a commit to ayojs/ayo that referenced this issue Oct 26, 2017

@starkwang @addaleax
buffer: buffer.transcode to use internal/errors 
`buffer.transcode` is still using raw TypeError. This change is to
convert it to use internal/errors.

Ref: nodejs/node#11273
PR-URL: nodejs/node#16352
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
2603b69

@reviewher reviewher referenced this issue Nov 13, 2017

Closed

node 9 loses error call site information in Buffer.concat #16994

addaleax added a commit to ayojs/ayo that referenced this issue Dec 7, 2017

@starkwang @addaleax
buffer: buffer.transcode to use internal/errors 
`buffer.transcode` is still using raw TypeError. This change is to
convert it to use internal/errors.

Ref: nodejs/node#11273
PR-URL: nodejs/node#16352
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
1935a92
@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 16, 2017

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.

@BridgeAR BridgeAR closed this Dec 16, 2017

Error Codes automation moved this from Reference to Done Dec 16, 2017

@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 16, 2017

@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 joyeecheung referenced this issue Dec 16, 2017

Closed

Tracking issue: Migrate errors to internal/errors.js #17709

6 of 6 tasks complete

@MylesBorins MylesBorins referenced this issue Feb 18, 2018

Closed

Initiative refactor #446

@joyeecheung joyeecheung referenced this issue Mar 31, 2019

Open

errors in punycode.js: assign codes or leave them alone? #27023

@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member

commented Mar 31, 2019

I believe this issue is mostly done now except the punycode ones. I opened #27023 for that, so I am going to close this. Feel free to reopen if anyone thinks otherwise.

@joyeecheung joyeecheung reopened this Mar 31, 2019

@joyeecheung

This comment has been minimized.

Sign in to view
Copy link
Member

commented Mar 31, 2019

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

@joyeecheung joyeecheung closed this Mar 31, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.