-
-
Notifications
You must be signed in to change notification settings - Fork 180
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 TimeoutException for timeout #1995
Conversation
🦋 Changeset detectedLatest commit: 6b7e812 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
2c199af
to
ccf1e1d
Compare
ccf1e1d
to
6b7e812
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally like this change. Though it limits the use of optionFromOptional
for handling timeout
errors, I feel as though the explicitness is much better.
Will wait for @mikearnaldi and @tim-smart to also weigh in.
Type
Description
I was staring at a failing test shouting
NoSuchElementException
at me and it took me some time to realise it is actually a timeout.While I somewhat get the point that being able to easily convert such an error to
Option
usingEffect.optionFromOptional
is convenient, I believe it is quite confusing. In such a case using explicitEffect.catchTag("TimeoutException", () => Effect.succeedNone)
and communicating clearly what's happening to a reader of my code is IMHO better.