tools: enforce `throw new Error()` with lint rule #3714

Trott · 2015-11-08T17:58:26Z

@evanlucas left a comment on another PR pointing out that the convention in the code base is throw new Error() over throw Error(). This PR changes the five instances of throw Error() in the code base and enforces it with a linting rule.

cjihrig · 2015-11-08T18:29:58Z

tools/eslint-rules/new-with-error.js

+
+    'ThrowStatement': function(node) {
+      if (node.argument.type === 'CallExpression') {
+        if (node.argument.callee.name === 'Error') {


These can be combined into a single if.

Done! Thanks!

evanlucas · 2015-11-08T20:48:23Z

LGTM

cjihrig · 2015-11-08T20:51:14Z

tools/eslint-rules/new-with-error.js

+  return {
+    'ThrowStatement': function(node) {
+      const arg = node.argument;
+      if ((arg.type === 'CallExpression') && (arg.callee.name === 'Error')) {


Small nit, but I don't think either set of internal parens are necessary.

~~/^[A-Z\w]*Error^/.test(arg.callee.name) should catch all other error types.~~

Edit: not quite. Should be: start - optional capital followed by \w* - Error - end, I think.

@cjihrig I'll make that adjustment.

@Fishrock123 You're after this: /^([A-Z]\w*)?Error$/ That assumes FooError() will never be the name of a function that returns an Error object and will only be a constructor for an Error object.

cjihrig · 2015-11-08T20:52:16Z

LGTM. Is it possible to extend this rule to the other built in error types (TypeError, RangeError, etc)?

Fishrock123 · 2015-11-08T21:22:25Z

LGTM, we should add the other error types too though. maybe including custom errors?

cjihrig · 2015-11-08T21:28:54Z

Maybe the types that the rule applies to should be a configuration option. Then you just have to check if arg.callee.name is in an array.

Trott · 2015-11-08T21:51:49Z

There are only five Error types listed in the docs so we could just hard-code those, allow them to be passed as options, or go with the regular expression solution above (/^([A-Z]\w*)?Error$/). I don't have a problem with extending it this way but I also want to avoid getting mired in bikeshedding. I'm inclined to just slap in the regular expression and we can always switch to a specified list later if we want to. Thoughts?

EDIT: Actually any of the three solutions seem fine to me. I don't care, as long as there's consensus from everyone else. I think any of them will work just fine for our situation.

cjihrig · 2015-11-08T23:07:31Z

I would prefer the array config solution because then the rule can be more easily extended ~~to things other than errors (new Socket(), new Server(), etc)~~. I think a regex would be more limiting in that way, and, of course, less readable.

mscdex · 2015-11-08T23:34:52Z

Maybe an array of strings and/or regexps? I think some simple regexps (e.g. /^(Type|Syntax|Range|Assertion|)Error$/) would be beneficial to prevent a huge list with many similar/related names.

cjihrig · 2015-11-08T23:41:23Z

@mscdex suggestion sounds like a winner to me.

Trott · 2015-11-09T00:20:57Z

I like the strings and regexp idea in theory, but in practice:

I don't think eslint can accept regular expressions as arguments. The arguments are specified in JSON Schema. No existing eslint rules accept regular expressions in the config file. The primitive types accepted are boolean, integer, number, string. I suppose we could accept strings and translate them into regular expressions, but yuck.
If they are all strings, it's an easy check using indexOf(). If there are regular expressions and strings mixed together, then a type check has to happen on each element. Not the end of the world, but some added complexity for functionality we may not need or use.

I'm inclined to go with an array of strings, at least initially.

Trott · 2015-11-09T00:27:55Z

Pushed a new commit with the "accept an array of Error types to check" feature. PTAL

cjihrig · 2015-11-09T00:35:12Z

I don't think eslint can accept regular expressions as arguments.

You're right. ESLint does the string to regex conversion instead.

cjihrig · 2015-11-09T16:17:19Z

tools/eslint-rules/new-with-error.js

+module.exports.schema = {
+  'type': 'array',
+  'items': [
+  {


Can you indent this and the following two lines?

cjihrig · 2015-11-09T16:19:13Z

LGTM. I'm fine with merging the rule with its current functionality.

jasnell · 2015-11-09T19:27:26Z

LGTM

Trott · 2015-11-10T15:32:01Z

@sindresorhus I proposed it but it was rejected.

I guess the next best thing would be to publish as a plugin so others can use it in their own projects. I'll get on that...

cjihrig · 2015-11-10T16:05:30Z

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

Trott · 2015-11-10T16:59:07Z

@sindresorhus A first pass is now available: https://www.npmjs.com/package/eslint-plugin-new-with-error

The documentation (and probably implementation) are lacking, but feel free to open issues left and right and that should be sufficient to shame me into improving it. :-D

In preparation for a lint rule that will enforce `throw new Error()` over `throw Error()`, fix the handful of instances in the code that use `throw Error()`.

Add linting rule requiring `throw new Error()` over `throw Error()`.

Trott · 2015-11-11T04:27:17Z

Rebased and squashed down to two commits (one for the code changes and a second for the lint rule itself). One more CI before landing: https://ci.nodejs.org/job/node-test-pull-request/698/

jbergstroem · 2015-11-11T04:57:42Z

Never mind the ubuntu1410 slave fail. I removed it while your tests were running.

In preparation for a lint rule that will enforce `throw new Error()` over `throw Error()`, fix the handful of instances in the code that use `throw Error()`. PR-URL: nodejs#3714 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add linting rule requiring `throw new Error()` over `throw Error()`. PR-URL: nodejs#3714 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Trott · 2015-11-11T05:02:33Z

Landed in 35f2f64 and 25cd455

In preparation for a lint rule that will enforce `throw new Error()` over `throw Error()`, fix the handful of instances in the code that use `throw Error()`. PR-URL: #3714 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add linting rule requiring `throw new Error()` over `throw Error()`. PR-URL: #3714 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

In preparation for a lint rule that will enforce `throw new Error()` over `throw Error()`, fix the handful of instances in the code that use `throw Error()`. PR-URL: #3714 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add linting rule requiring `throw new Error()` over `throw Error()`. PR-URL: #3714 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

In preparation for a lint rule that will enforce `throw new Error()` over `throw Error()`, fix the handful of instances in the code that use `throw Error()`. PR-URL: #3714 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add linting rule requiring `throw new Error()` over `throw Error()`. PR-URL: #3714 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Trott added the tools Issues and PRs related to the tools directory. label Nov 8, 2015

cjihrig reviewed Nov 8, 2015
View reviewed changes

cjihrig reviewed Nov 9, 2015
View reviewed changes

Trott force-pushed the new-error branch from 1678edb to 1774e79 Compare November 9, 2015 17:12

Trott added 2 commits November 10, 2015 18:06

tools: Use throw new Error() consistently

a9b3b86

In preparation for a lint rule that will enforce `throw new Error()` over `throw Error()`, fix the handful of instances in the code that use `throw Error()`.

tools: enforce throw new Error() with lint rule

97ed5ec

Add linting rule requiring `throw new Error()` over `throw Error()`.

Trott force-pushed the new-error branch from 1774e79 to 97ed5ec Compare November 11, 2015 02:08

Trott closed this Nov 11, 2015

Fishrock123 mentioned this pull request Nov 11, 2015

Propose v5.1.0 #3736

Merged

rvagg mentioned this pull request Dec 17, 2015

Release 4.2.4 Planning #4321

Closed

jasnell added the lts-watch-v4.x label Dec 17, 2015

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Dec 29, 2015

MylesBorins mentioned this pull request Jan 19, 2016

V4.2.5 proposal #4768

Merged

Trott deleted the new-error branch January 13, 2022 22:29

aliscco mentioned this pull request Dec 3, 2023

[Snyk] Security upgrade lerna from 3.22.1 to 7.0.0 aliscco/alisco-node#1113

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: enforce `throw new Error()` with lint rule #3714

tools: enforce `throw new Error()` with lint rule #3714

Trott commented Nov 8, 2015

cjihrig Nov 8, 2015

Trott Nov 8, 2015

evanlucas commented Nov 8, 2015

cjihrig Nov 8, 2015

Fishrock123 Nov 8, 2015

Trott Nov 8, 2015

cjihrig commented Nov 8, 2015

Fishrock123 commented Nov 8, 2015

cjihrig commented Nov 8, 2015

Trott commented Nov 8, 2015

cjihrig commented Nov 8, 2015

mscdex commented Nov 8, 2015

cjihrig commented Nov 8, 2015

Trott commented Nov 9, 2015

Trott commented Nov 9, 2015

cjihrig commented Nov 9, 2015

cjihrig Nov 9, 2015

cjihrig commented Nov 9, 2015

jasnell commented Nov 9, 2015

Trott commented Nov 10, 2015

cjihrig commented Nov 10, 2015

Trott commented Nov 10, 2015

Trott commented Nov 11, 2015

jbergstroem commented Nov 11, 2015

Trott commented Nov 11, 2015

tools: enforce throw new Error() with lint rule #3714

tools: enforce throw new Error() with lint rule #3714

Conversation

Trott commented Nov 8, 2015

cjihrig Nov 8, 2015

Choose a reason for hiding this comment

Trott Nov 8, 2015

Choose a reason for hiding this comment

evanlucas commented Nov 8, 2015

cjihrig Nov 8, 2015

Choose a reason for hiding this comment

Fishrock123 Nov 8, 2015

Choose a reason for hiding this comment

Trott Nov 8, 2015

Choose a reason for hiding this comment

cjihrig commented Nov 8, 2015

Fishrock123 commented Nov 8, 2015

cjihrig commented Nov 8, 2015

Trott commented Nov 8, 2015

cjihrig commented Nov 8, 2015

mscdex commented Nov 8, 2015

cjihrig commented Nov 8, 2015

Trott commented Nov 9, 2015

Trott commented Nov 9, 2015

cjihrig commented Nov 9, 2015

cjihrig Nov 9, 2015

Choose a reason for hiding this comment

cjihrig commented Nov 9, 2015

jasnell commented Nov 9, 2015

Trott commented Nov 10, 2015

cjihrig commented Nov 10, 2015

Trott commented Nov 10, 2015

Trott commented Nov 11, 2015

jbergstroem commented Nov 11, 2015

Trott commented Nov 11, 2015

tools: enforce `throw new Error()` with lint rule #3714

tools: enforce `throw new Error()` with lint rule #3714