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

Crash Error instances without processing them #31

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

konker
Copy link
Contributor

@konker konker commented Sep 15, 2022

This PR allows instances of errors to pass-through the crash helper untouched.
This removes a layer of processing when the error case is already an Error instance.

Additionally, some fixes were made to incorrect async usage of jest resolves/rejects statements.
E.g.

it('should work', () => {
    expect(ruins.fromTaskOption(...)).resolves.toBe(...);
}

Changed to:

it('should work', async () => {
    await expect(ruins.fromTaskOption(...)).resolves.toBe(...);
}

This uncovered a broken test for task-option, which was also fixed.

@konker konker changed the title Crash errors Crash Error instances without processing them Sep 15, 2022
@konker konker requested a review from cyberixae September 15, 2022 14:43
@cyberixae cyberixae closed this Sep 16, 2022
@cyberixae cyberixae reopened this Sep 16, 2022
@cyberixae
Copy link
Contributor

The way ruins throws errors at the moment is clearly not ideal. I think there are two competing goals here. One is throwing the error type E as accurately as possible. This seems particularly useful for situations where the type E has been purposefully designed to be thrown. The other one is wrapping E into a RuinsError that makes sure the exception has a proper stack trace and remains distinguishable with instanceof in situations where the type E has not been purposefully designed with throwing in mind. In the latter case, I think it would be nice if the original error E was somehow recoverable from a RuinsError.

Ruins was originally developed for use in situations where the type E has not been purposefully been designed for use with Ruins. However, I do see some value in the ability to create interfaces purposefully design to interact in a certain way when used together with Ruins. I am wondering whether or not a separate fromEitherError function would make sense. It could limit input values to Either<Error, T> values that have should already be suitable for throwing.

Whether or not Ruins should have special support for values specifically handcrafted with Ruins in mind is debatable. Perhaps a separate package could be created for utilities that require the input types to be crafted in a certain way. I feel like there is a slight difference even though both use cases are part of the same puzzle.

Copy link
Contributor

@cyberixae cyberixae left a comment

Choose a reason for hiding this comment

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

Should we close this?

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.

2 participants