supermacro / neverthrow

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

[Feature request] `safeTry` should not require `.safeUnwrap()` #580

Closed mattpocock closed 1 month ago

mattpocock commented 2 months ago

One of neverthrow's weakest API's is safeTry, combined with .safeUnwrap

const result = safeTry(function*() {
  const foo = yield* ok("foo").safeUnwrap();
});

I propose, instead, that .safeUnwrap() should be called whenever a Result is yield*-ed.

const result = safeTry(function*() {
  const foo = yield* ok("foo");
});

This matches the similar API found in Effect.gen.

To make this work, Result and ResultAsync should expose a [Symbol.iterator] which calls .safeUnwrap. Since it's rare that anyone would want to iterate over a Result, this API feels about as safe as .safeUnwrap().

m-shaka commented 2 months ago

I've tried to implement [Symbol.iterator] before, but there is an issue. Comparing two Err raises Maximum call stack size exceeded, I'm not sure why though.

image

You can try https://github.com/supermacro/neverthrow/compare/symbol-iterator?expand=1&w=1 (I forked #571 to use vitest)

mattpocock commented 2 months ago

@m-shaka The fix for this is to add this to the Err class:

[Symbol.iterator](): Generator<Err<never, E>, T> {
  const self = this

  return (function* () {
    yield self

    return self
  })() as any
}

The issue appeared to be caused by infinite recursion of err calls when the symbol was iterated over. Using this instead ends the recursion loop.

BUT this raises an issue - I can't see a way to do two things at once:

I'm not sure how often this would come up outside of a test environment, though - how often are you iterating over Results?

m-shaka commented 2 months ago

Lovely! I think it's ok to give up this one. We'd need a comment about why return is necessary here, though

Only permit that iterator to be called within .safeTry

@supermacro What do you think?

faergeek commented 2 months ago

I was recently experimenting with similar kinds of stuff (after discovering a reader, but that's besides the point xD).

I haven't tried using neverthrow myself yet and don't know much about it, but since the idea is basically the same I just wanted to share how I implemented generators/iterators in a similar either/result monad. In my case I have a single class for success/failure branches and a discriminated union inside of it, which is a bit different from neverthrow, but otherwise the idea is pretty much the same.

Here's the whole implementation of generators/iterators:

https://github.com/faergeek/monads/blob/experiments/src/result.ts#L31-L45

And here is a test serving as a usage example. It allows to yield* result objects in a sequence and stop on a first result which is a failure and return that failure:

https://github.com/faergeek/monads/blob/experiments/src/result.spec.ts#L26-L70

It also runs whatever is in a try/finally block as can be seen in a test, since it calls .return on a generator.

In my implementation in [Symbol.iterator] I use yield for a failure and return for a success. I found this way easier for types to be inferred. Just E nicely turns into a union in case of multiple different types of errors. I haven't found a way to make multiple Result<never, "whatever"> to turn into a union like that. And when calling .return it's being wrapped into a Result.Err again, which sucks, but it doesn't seem to affect performance as much as just the generators themselves.

I couldn't believe it could be implemented in such a simple way and was very excited and wanted to combine it with async support and dependency injection and use it all over the place before I tried to measure it's performance. Generators turn out to be pretty slow unfortunately. Benchmark code in my repo is here https://github.com/faergeek/monads/blob/experiments/src/result.bench.ts

Here's a screenshot of pnpm run benchmark run:

img

I'm not entirely sure if my benchmarks are 100% correct, I'm not very experienced in that. Did anyone measure how generators affect performance in neverthrow?

mattpocock commented 2 months ago

@faergeek Could you post this in a separate issue? It's interesting, but not sure how it's relevant to the topic being discussed

faergeek commented 2 months ago

@mattpocock sorry, I didn't really mean to write so much about try/finally, performance and all of that, these are different topics for sure.

I just wanted to point out that to implement [Symbol.iterator] with generators there's no need to create an IIFE (IIGFE?) like in your comment or in a branch linked in a comment before it.

This works:

  /* eslint-disable-next-line require-yield */
  *[Symbol.iterator]() {
    return this.value
  }
*[Symbol.iterator](): Generator<Err<never, E>, T> {
  yield this
  return this as any
}

Isn't it more convenient and easy to understand? And, by the way, it's also faster, but I'll stop right here this time :-)