-
Notifications
You must be signed in to change notification settings - Fork 207
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
Fix typings for unfold #506
base: master
Are you sure you want to change the base?
Conversation
Hey @eddiew, thanks. Hmmm, yes, the type seems wrong. I like your idea to introduce an type UnfoldValue<S, V> = { done: false, seed: S, value: V } | { done: true, value?: V } The first variant could use an intersection with the existing SeedValue type--that'd be fine. It needs to have a type UnfoldValue<S, V> = { done: false } & SeedValue<S, V> | { done: true, value?: V } I think using an intersection is a bit harder to read, though. What do you think? |
I thought
But if you'd rather keep the |
Maybe also add transduce to definitions? |
Sorry for taking a while to respond!
Hmmm, yes, the current docs do indeed read that way, and so does the example. Ok, let's go with allowing done to be missing or undefined. Thanks for pointing that out. |
@MaxSvargal Let's do that in another PR. See also: #512 where it's being discussed. |
type-definitions/most.d.ts
Outdated
@@ -1,4 +1,5 @@ | |||
declare type SeedValue<S, V> = { seed: S, value: V }; | |||
declare type UnfoldValue<S, V> = SeedValue<S,V> | { done: boolean } |
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.
Since the docs are kind of loose, we may need to be even more permissive. Maybe we can go with this, and then relax it if it causes someone some pain.
Seems like we should tighten the done case to { done: true }
since it'd be pointless to return { done: false }
. What do you think?
Good catch. I updated the code to require |
Summary
The typings for Most.unfold don't match what the code does or the API docs, so I fixed them.