Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Axios's toJSON method to make shorter errors (#632) #662

Merged
merged 1 commit into from
Mar 13, 2023

Conversation

BryceStevenWilley
Copy link
Collaborator

Accidentally merged #632 into releases/v4 instead of v4 (the PR pre-dated v4 as the default branch), so this process is backwards; but here's the changes cherry-picked from releases/v4.

  • Use Axios's toJSON method to make shorter errors

On an example erroring docassemble server (for example, this server returning a 502 to a delete project), the entire axios error will print, which is that case in 1.8k lines, most of it being garbage.

Axios has a toJSON() method on errors that should reduce length of the errors being shown, as it will not print the most annoying parts of the long errors, like the internal Axios socket objects

Still need to test, as it's difficult to get a server to 502 on demand.

Fix #605.

  • Actually fail when we log the errors
  • Move toJSON() calls to docassemble_api_REST
  • Add better comment to process.exitCode

* Use Axios's `toJSON` method to make shorter errors

On an example erroring docassemble server (for example, this server returning
a [502 to a delete project](https://github.com/SuffolkLITLab/docassemble-EFSPIntegration/actions/runs/3641406524/jobs/6147303425)),
the entire axios error will print, which is that case in 1.8k lines, most of it
being garbage.

Axios has a `toJSON()` method on errors that should reduce length of the errors
being shown, as it will not print the most annoying parts of the long errors,
like the internal Axios socket objects

Still need to test, as it's difficult to get a server to 502 on
demand.

Fix #605.

* Actually fail when we log the errors
* Move `toJSON()` calls to docassemble_api_REST
* Add better comment to `process.exitCode`

Co-authored-by: plocket <52798256+plocket@users.noreply.github.com>
Copy link
Collaborator

@plocket plocket left a comment

Choose a reason for hiding this comment

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

Great! Thank you! You can take or leave the comment.

<!-- ## [Unreleased] -->
## [Unreleased]
### Changed
- Shorten Axios errors to make them more readable (https://github.com/SuffolkLITLab/ALKiln/pull/632)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the issue number isn't what you intended?

Suggested change
- Shorten Axios errors to make them more readable (https://github.com/SuffolkLITLab/ALKiln/pull/632)
- Shorten Axios errors to make them more readable (https://github.com/SuffolkLITLab/ALKiln/pull/605)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also liked the list of changes in the comment so I put them here, but if you don't want to include that, that's fine. Just an alternative version.

Suggested change
- Shorten Axios errors to make them more readable (https://github.com/SuffolkLITLab/ALKiln/pull/632)
- Shorten Axios errors to make them more readable (https://github.com/SuffolkLITLab/ALKiln/pull/605)
- Actually fail when we log the errors
- Move `toJSON()` calls to docassemble_api_REST
- Add better comment to `process.exitCode`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do intentionally link to the PR when I fix things. Issues usually have a lot of extra discussion that don't get implemented, and the issue is still linked to from the PR, usually in the first comment. I've found it's a lot easier to find the issue fixed from a PR than finding the PR from the issue.

I normally would be fine with adding more details to the changelog, but those specific commit comments are very internal, and don't make sense outside the context of the choices made in the PR ("move toJSON calls" move them from where? from a different file they were in earlier in the PR. From the changelog comment, it looks like those toJSON calls were around before that release, but they weren't).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Those make some sense. I'm good with whatever you choose.

@plocket
Copy link
Collaborator

plocket commented Mar 11, 2023

Also, do you think this also handles #550 sufficiently?

@BryceStevenWilley
Copy link
Collaborator Author

TBH, I don't think it addresses #550, at least not the first part. I'll mark the second part as being done there.

@BryceStevenWilley BryceStevenWilley merged commit 9c0f2a0 into v4 Mar 13, 2023
@BryceStevenWilley BryceStevenWilley deleted the axios_error_v4 branch March 13, 2023 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove axios full error log for setup (e.g. when Project is deleted or created), like with a 504
2 participants