Shields: throw or throw new?

[PyvesB]

I noticed whilst debugging #3035 that we were incorrectly using throw InvalidResponse. This results in the following error:
Uncaught TypeError: Class constructor InvalidResponse cannot be invoked without 'new'

I didn't fix it as part of that PR as I thought there may be other similar occurrences and was planning on fixing them all in one pass, but it turns out that this is the only problematic call.

However, we are quite inconsistent in the way we throw exceptions, as the standard ones accept both syntaxes (i.e. both throw Error and throw new Error are valid and should be equivalent according to the specification). We have 45 occurrences of throw X and 93 occurrences of throw new X in our current codebase.

Should we:

• enforce the use of throw new X using this ESLint plugin or similar? We can specify custom exception names which would prevent that one bug from being introduced again.
• modify our custom exceptions to add a factory method so that they can be called without new and consequently drop the new keyword everywhere?
• not worry too much and just fix that one bug we currently have?
• do something else? After all, as we improve our code coverage we will reduce the risk of these mistakes slipping through.

I'm happy to tackle this once we've reached an agreement. 👍

[paulmelnikow]

I tend to use throw Error because as you point out they are equivalent according to the spec. I'd be all for linting throw new X for the others!

Do you know if ESLint's new-cap rule can be used for this?

[PyvesB]

new-cap seems to be for something else, i.e. casing of the first letter of the constructor. I didn't find any standard ESLint rules that seemed relevant, I think we would have to use an additional plugin like the one I suggested.

[paulmelnikow]

InvalidResponse() needs to be invoked with new, which I think is an equivalent way of stating the problem. It looks like "capIsNew": true might do what we want. and capIsNewExceptions could support overriding.

[chris48s]

Yeah this is one of those mental javascript foot-guns. Lets get a linter rule in for it. I'm in favour of using eslint-plugin-new-with-error to enforce throw new in all cases.

[PyvesB]

InvalidResponse() needs to be invoked with new, which I think is an equivalent way of stating the problem. It looks like "capIsNew": true might do what we want. and capIsNewExceptions could support overriding.

I had not spotted those sub-options, let me take a look.

[PyvesB]

So it does seem to work as expected, with one small subtlety: Error is part of a CAPS_ALLOWED list, so is not covered by this rule.

This means that @paulmelnikow won't be happy, because we can't enforce only having throw Error and @chris48s won't be happy because we can't enforce only having throw new Error. 😄

We will have to keep a mixture of both just for Error (and the other items in CAPS_ALLOWED), however, this will successfully catch bugs such as throw InvalidResponse. We will only be left with a minor style inconsistency, but in my opinion it's an acceptable trade-off and does not justify pulling in a new plugin.

Will shortly submit a pull request with my progress so far, but of course we can continue the discussion if need-be!

[PyvesB]

I've completed this quite fast, but if other maintainers think the linting should be stricter or looser or any different, please feel free to voice your concerns!