[cdnjs gem] make exception classes more consistent

[chris48s]

As I've been working on the new service code, I've had a couple of thoughts on the exception classes:

#1590 (comment)
#1680 (comment)

I think its worth looking at this before we are using this code in too many places and it becomes difficult to change the constructor signatures. This PR proposes the following changes:

• All exception classes now have the same signature. I find this confusing:

class InvalidResponse extends Error {
  constructor(prettyMessage = 'invalid', underlyingError) {
    // ...
  }
}

class Inaccessible extends Error {
  constructor(underlyingError, prettyMessage = 'inaccessible') {
    // ...
  }
}

• Passing a props object means underlyingError and prettyMessage are always optional. This means we don't need to use workarounds like throw new InvalidResponse(undefined, err); or throw new InvalidResponse('invalid', new Error()); if we don't want to customise the error or don't have an exception to wrap.

• Declare a base class which our runtime exceptions inherit from

does this seem like a reasonable approach?

[shields-ci]

	Warnings
⚠️	This PR modified helper functions in lib/ but not accompanying tests. That's okay so long as it's refactoring existing code.

	Messages
📖	✨ Thanks for your contribution to Shields, @chris48s!

Generated by 🚫 dangerJS

[PyvesB]

Looks overall good to me! 👍

Minor observation: should we keep a more informative message for the InvalidResponses in gem.js ("version is null", "info is invalid")?

[chris48s]

I was in 2 minds about whether it is useful to wrap a new Error() when we're not handling anything, but you're right that it allows us to provide a more detailed message.

[RedSparr0w]

Looks good to me!
Left a couple of notes on some things i wasn't sure about though.

[RedSparr0w]

should this line (& line below) be using this.defaultPrettyMessage in place of 'not found'

[chris48s]

That would be ideal, but there's a catch 22:

If we try to reference this before super(), error 'this' is not allowed before 'super()' is thrown (which makes sense), but we need to assemble message before we can call super().

Maybe an alternative would be to declare the default messages as constants outside the classes so we can say

const defaultNotFoundMessage = 'not found';

class NotFound extends ShieldsRuntimeError {

  get name() { return 'NotFound'; }
  get defaultPrettyMessage() { return defaultNotFoundMessage; }

  constructor(props = {}) {
    const prettyMessage = props.prettyMessage || defaultNotFoundMessage;
    //...
  }
}

do you think that is better?

[RedSparr0w]

Ah yeah that makes sense, sounds good to me 👍
You can keep it how it is or change it, whichever makes the most sense for you.

[RedSparr0w]

would we be better to remove this line and declare the default value in the header instead?

constructor(props = {}) {

[chris48s]

👍 updated