Skip to content
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
79 of 80 tasks
jasnell opened this issue Feb 9, 2017 · 48 comments
Closed
79 of 80 tasks

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

jasnell opened this issue Feb 9, 2017 · 48 comments
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. meta Issues and PRs related to the general management of the project.

Comments

@jasnell
Copy link
Member

jasnell commented Feb 9, 2017

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 Issues and PRs related to JavaScript errors originated in Node.js core. label Feb 9, 2017
@fl0w
Copy link

fl0w 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
Copy link
Member

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
Copy link
Member

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

@jasnell
Copy link
Member Author

jasnell 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!

@seppevs
Copy link
Contributor

seppevs 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
Copy link
Member Author

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

@joyeecheung
Copy link
Member

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

@jasnell
Copy link
Member Author

jasnell commented Feb 11, 2017

We should, yes.

Qard pushed a commit to Qard/ayo that referenced this issue Sep 21, 2017
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>
@BridgeAR
Copy link
Member

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
Copy link
Contributor

I am on "querystring.js"

@ramimoshe
Copy link
Contributor

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

jasnell pushed a commit to pmatzavin/node that referenced this issue Sep 25, 2017
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
joyeecheung pushed a commit that referenced this issue Oct 15, 2017
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>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 15, 2017
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>
starkwang added a commit to starkwang/node that referenced this issue Oct 22, 2017
`buffer.transcode` is still using raw TypeError. This change is to
convert it to use internal/errors.

Ref: nodejs#11273
gireeshpunathil pushed a commit that referenced this issue Oct 23, 2017
`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>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 26, 2017
`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>
addaleax pushed a commit to ayojs/ayo that referenced this issue Dec 7, 2017
`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>
@BridgeAR
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

No branches or pull requests