Skip to content
This repository has been archived by the owner on Jan 24, 2023. It is now read-only.

Get the full, descriptive error response returned from skyd #351

Merged
merged 3 commits into from
Dec 13, 2021

Conversation

mrcnski
Copy link
Contributor

@mrcnski mrcnski commented Dec 3, 2021

PULL REQUEST

Overview

Previously, methods were returning error messages that said e.g. "Request failed
with status code 429" and we had to drill down into the actual response to see
what the skyd error was.

All methods that make a request now return a descriptive error message based on
the skyd response message.

  • Added ExecuteRequestError which contains the original Axios error. We now
    throw this on network errors instead of AxiosError directly.

  • Added request.ts file for ExecuteRequestError. I originally tried putting it in
    client.ts where it was causing circular dependencies and stuff not being defined
    at runtime.

  • Also, I fixed a bug where a promise in uploadLargeFile was not being
    rejected on error.

Checklist

Review and complete the checklist to ensure that the PR is complete before assigned to an approver.

  • All new methods or updated methods have clear docstrings
  • Testing added or updated for new methods
  • Verify if any changes impact the WebPortal Health Checks
  • Approriate documentation updated
  • Changelog file created

Previously, methods were returning error messages that said e.g. "Request failed
with status code 429" and we had to drill down into the actual response to see
what the skyd error was.

All methods that make a request now return a descriptive error message based on
the skyd response message.

- Added ExecuteRequestError which contains the original Axios error

- Added request.ts file for ExecuteRequestError, where it was causing circular
  dependencies and stuff not being defined at runtime.

- Also, I fixed a bug where a promise in `uploadLargeFile` was not being
  rejected on error.
@mrcnski mrcnski added enhancement New feature or request refactor labels Dec 3, 2021
peterjan
peterjan previously approved these changes Dec 10, 2021
Copy link

@peterjan peterjan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approved seeing as the code likely works. I do think some of the suggestions I made are worth implementing though.

src/request.ts Outdated Show resolved Hide resolved
src/registry.ts Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
integration/skydb.test.ts Show resolved Hide resolved
@mrcnski mrcnski merged commit 753a430 into master Dec 13, 2021
@kwypchlo kwypchlo deleted the full-skyd-error branch January 11, 2022 10:29
@mrcnski mrcnski mentioned this pull request Apr 4, 2022
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants