feat!: Refactor error handling

[joe-yeager]

Description and Context

Note: This will be a breaking change, so this PR is targeting the next branch and will be in the next major

Whats changed:

Removals

• Removed all thrower methods because they were thin wrappers obscure our intent by adding an additional layer in front of the throw. Explicitly throwing the error makes our intentions clearer, and actually makes our code easier to statically analyze. The change uncovered 2 cases where we are calling throwX in a try block, which then obscures the intended error message with a new more generic error message. Methods removed:
  • throwError
  • throwErrorWithMessage
  • throwApiError
  • throwApiUploadError
  • throwValidationError
  • throwAuthErrorWithMessage
  • throwFileSystemError
• Removes HubSpotAuthError, because we now have HubSpotHttpError and we can check if it is an auth error using isAuthError in the errors module
• parseValidationErrors is no longer exported
• getHubSpotHttpErrorWithContext is no longer exported

Additions

• Creates a new Error object FileSystemError to replace the need for throwFileSystemError

Changes

• Compresses apiErrors, standardErrors, and fileSystemErrors into a single errors module.
• Moves all of the logic for extraction of error messages from http responses into the HubSpotHttpError class and makes it an automatic operation.
  • This means the HubSpotHttpError with have the user friendly http error message in it's message property automatically.
  • The extraction of the context object is automatic on instantiation of the HubSpotHttpError, but can be overridden

Screenshots

TODO

• Finish unit testing
• FInish integration with CLI: v2 LDL hubspot-cli#1118
• Update errors/README.md

Who to Notify

@brandenrodgers @camden11 @kemmerle

[brandenrodgers]

This is a great example of the value of this 🎉

[brandenrodgers]

I think we might need to dig into this parse validation util. I noticed that we're logging out some of the important context, but not all of it. For example:

Here's what logs out when I get a "missing scopes" error response. I can get this by trying to run this command against my developer portal, which doesn't have the right content scopes to access CMS data.

It's trying to tell me the scopes I'm missing, but it's not including all of the information that I need. This is the shape of responseData when that happens:

context here contains the scopes, like this: ["content", "content"] (not sure why it's in there twice)

---

Also, from what I can tell, in the previous version of the error handling we only called it in a few places (throwApiUploadError is the only util that called isApiUploadValidationError, which ran this validation parsing logic). I wonder if it will have an impact on some of the existing error messages?

[joe-yeager]

I added a method to log the formattedValidationErrors that the CLI now calls if the error is a validation error.

I'll look into using the context object. How did you trigger missing scopes error? Just remove those scopes from the app?

[brandenrodgers]

The reference to "app" here is misleading. In reality the error is referencing the personal access key. So you can generate a personal access key that doesn't have any scopes, and then try to run various CLI commands.

[camden11]

This is looking great, really cleans things up nicely and should hopefully make the experience of using and contributing to local-dev-lib a lot better. I haven't tested yet, but I did my best to look over everything and didn't notice any problems. Are there any particular areas you need help with extra testing or that I should give a more focused look?

[joe-yeager]

Are there any particular areas you need help with extra testing or that I should give a more focused look?

@camden11 Since it is such a wide spreading change, I think we really need to try to touch as many places as possible, so just widespread general testing.

[camden11]

I still haven't had a chance to test this much, but assuming there will be plenty of time to text the next branch if its not going to be merged for a bit.

Looked everything over again and LGTM

[brandenrodgers]

This lgtm! I did some testing locally and it all seems to be WAD. We'll have to hammer on it some more since it has a wide impact, but this is good to go into the next branch 👌