supermacro / neverthrow

Type-Safe Errors for JS & TypeScript
MIT License
4.03k stars 84 forks source link

[Feature request] Add `unwrapOrThrow` #598

Closed Papooch closed 4 weeks ago

Papooch commented 1 month ago

Currently, we use a custom Result implementation with a very similar base API (but we'd like to migrate to this library).

When interfacing with a result-based code from non-result-based (one that relies on try/catch), throwing error is often has a legitimate use-case - for example, throwing a ConflictExeption causes the API layer to map this into a HTTP 409 error when called from HTTP context, or some other client-facing error when called from elsewhere.

We added a result.getOrThrow() method exactly for this purpose, as it's not practical to refactor the rest of the calling code to be result-aware.

This method either returns the Ok value, or throws the Err error

// proposed
return result.unwrapOrThrow()

// shorthand for
return result.match(
  (value) => value,
  (err) => { throw err }
)

An alternative name could be just unwrap (similar to Rust's naming), but I personally prefer the explicit OrThrow suffix, analogous to unwrapOr(something)

paduc commented 1 month ago

Hello @Papooch !

I think the neverthrow library should never throw. 😉

More seriously, you lose error types by throwing and errors types are the point of this library.

What would unwrapOrThrow bring you that the following code wouldn't ?

if(result.isErr()){
   throw result.error
}
Papooch commented 4 weeks ago

Yes, you lose error types, but this happens at the boundary between try/catch world and Result world, where the former doesn't care about error types anyway.

I know that strictly speaking, this is not a concern of the library, but it's nice convenience method that we found very useful in the real world, where codebases mix a lot of different paradigms.

Of course, it can be implemented in the userland with

function unwrapOrThrow<T>(result: Result<T, any>): T {
  return result.match(
    (value) => value,
    (err) => { throw err }
  )
}

but it breaks the pipe-like style of method chaining and you need to remember that it exists and to import it, which will inevitably lead to some parts of the code using if/throw and others this utility function.


The bottom line is - this method is useful not because it's correct, but because it's convenient in the real world.

paduc commented 4 weeks ago

Yes, you lose error types, but this happens at the boundary between try/catch world and Result world, where the former doesn't care about error types anyway.

Well you should care about the error types, that's the point.

but it breaks the pipe-like style of method chaining and you need to remember that it exists and to import it, which will inevitably lead to some parts of the code using if/throw and others this utility function.

When you step outside of the result world, you lose pipe-like chaining anyway.

The bottom line is - this method is useful not because it's correct, but because it's convenient in the real world.

It's probably convenient but to do something that smells bad to me.

Papooch commented 4 weeks ago

Fair enough, I would definitely be hesitant to add this to my library, too, if I hadn't explicitly dealt with the situation I described above.

supermacro commented 4 weeks ago

Why not use _unsafeUnwrap?

Papooch commented 4 weeks ago

@supermacro because it doesn't throw the wrapped error, but rather a custom object only meant for tests.

supermacro commented 4 weeks ago

I see, unfortunately this proposed design doesn't align with the goals of this lib