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

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

Open
mattpocock opened this issue Sep 7, 2024 · 6 comments · May be fixed by #589
Open

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

mattpocock opened this issue Sep 7, 2024 · 6 comments · May be fixed by #589

Comments

@mattpocock
Copy link
Contributor

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
Copy link
Collaborator

m-shaka commented Sep 9, 2024

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
Copy link
Contributor Author

@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:

  • Allow an iterator on Result which can be compared via .isEqual
  • Only permit that iterator to be called within .safeTry

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
Copy link
Collaborator

m-shaka commented Sep 10, 2024

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
Copy link

faergeek commented Sep 12, 2024

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
Copy link
Contributor Author

@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
Copy link

faergeek commented Sep 13, 2024

@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 :-)

@dmmulroy dmmulroy linked a pull request Oct 9, 2024 that will close this issue
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 a pull request may close this issue.

3 participants