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

feat: support non-standard errors #414

Merged
merged 8 commits into from
Nov 11, 2023
Merged

feat: support non-standard errors #414

merged 8 commits into from
Nov 11, 2023

Conversation

aradbar
Copy link
Contributor

@aradbar aradbar commented Jul 14, 2023

Resolves #411
Support non-standard errors. Errors with a read only "message" raise - "Cannot set property message of X which has only a getter"

Description:

Allows the library to handle errors that have been thrown in non standard ways.

Type of change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Why is this change required?:

Allows users to implement custom error handling solutions.

Code changes:

  • Adjusted the logic for error handling to use standardised functions toStandardError and toTimeoutError
  • Extends the formatted error with a cause containing the original error

@aradbar aradbar requested review from a team as code owners July 14, 2023 12:42
@aradbar aradbar changed the title Support non-standard errors feat: support non-standard errors Jul 14, 2023
@codeclimate
Copy link

codeclimate bot commented Jul 14, 2023

Code Climate has analyzed commit ab1c57f and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 98.2% (0.1% change).

View more on Code Climate.

src/consumer.ts Outdated Show resolved Hide resolved
@mhuggins
Copy link

Would love to see this land in the library! I just ran into this today and left a comment over here about how I had to work around it.

@nicholasgriffintn
Copy link
Member

nicholasgriffintn commented Nov 11, 2023

Sorry, this PR seems to have not been updated, as said previously, the tests appear to be passing but I would prefer the tests to be a little better, in particular, it's probably worth while adding E2E tests in for an implementation similar.

That said, this is probably fine as long as it actually solves the issue, I don't truly know as we don't have an example application for this issue.

@nicholasgriffintn nicholasgriffintn merged commit 0b4d5d7 into bbc:main Nov 11, 2023
7 checks passed
@mhuggins
Copy link

Thank you!

Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Support none standard errors - "Cannot set property message of which has only a getter"
4 participants