cujojs / most

Ultra-high performance reactive programming
MIT License
3.49k stars 231 forks source link

Fix typings for unfold #506

Open eddiew opened 6 years ago

eddiew commented 6 years ago

Summary

The typings for Most.unfold don't match what the code does or the API docs, so I fixed them.

briancavalier commented 6 years ago

Hey @eddiew, thanks. Hmmm, yes, the type seems wrong. I like your idea to introduce an UnfoldValue sum type. How about this?

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 done: boolean, so maybe like:

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?

eddiew commented 6 years ago

I thought done was allowed to be undefined if not true, so then it wouldn't be necessary to have the done: false in the first variant. In that case we could do:

type UnfoldValue<S, V> = SeedValue<S, V> | { done: true, value?: V }

But if you'd rather keep the done: false then I agree that the intersection is a little bit confusing, and I like the first version you have better.

MaxSvargal commented 6 years ago

Maybe also add transduce to definitions?

briancavalier commented 6 years ago

Sorry for taking a while to respond!

I thought done was allowed to be undefined if not true

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.

briancavalier commented 6 years ago

@MaxSvargal Let's do that in another PR. See also: https://github.com/cujojs/most/issues/512 where it's being discussed.

eddiew commented 6 years ago

Good catch. I updated the code to require done: true