calmm-js / karet.util

Utilities for working with Karet
MIT License
20 stars 5 forks source link

Curried Kefir.combine #5

Open rikutiira opened 7 years ago

rikutiira commented 7 years ago

Before doing a PR about this feature, I'd like to discuss how curried Kefir.combine should be implemented in Karet Util.

Personally, I believe this would be the best approach:

U.combine(['foo'], value)
U.combinePassive(['bar'], value)

U.seq(value,
    U.combine(['foo']),
    U.combinePassive(['bar'])
)

But I'd like to double-check if this approach is OK with you @polytypic ? It's worth discussing over because:

1) There is no longer a single function you can combine active and passive values with. Personally I think it's not a problem.

2) The bigger issue might be that it's inconsistent with how Kefir.merge is currently implemented as U.parallel, it's not curriable and therefore not directly usable when piping functions like with U.seq. But on the other hand, you can't simply write U.combine([...observables]) with this suggested approach.

Any opinions?

polytypic commented 7 years ago

Hmm...

Does the first argument need to be an array and what does the output look like with U.combine(['foo'], value)?

How about naming this combine variant slightly differently. Perhaps something like:

U.seq(value,
      U.andCombine([foo]),
      U.andCombinePassive([bar]))

What would the output be like (a nested array?) in the above with two uses?

There could be a similar variant for merge (named andParallel).

BTW, in Partial Lenses there is currently an L.orElse function for use as .reduceRight callback and for curried use with U.seq, and L.choice (and upcoming L.choices) for cases where you can list all the variants directly.

rikutiira commented 7 years ago

Yeah, the first argument would have to be an array (unless you want to support multiple types), and the output would be [value, 'foo'].

I like the idea of slightly different naming. So basically we'd have:

U.combine
U.andCombine
U.andCombinePassive

Of which the latter 2 are curried and U.combine works like U.parallel at the moment. I'd also consider if U.andCombine should be named U.andCombineActive for clarity, though.

But hmm... if you can already use partial.lenses to do similar things, maybe it's worth consideration to not add these variants of combine and only have non-curried version of combine because I feel that having 3 different variants of combine can be quite confusing. Although it would also be nice to be able to curry without having to use partial.lenses since it's quite large library to get familiar with. But you could always use U.flatMapLatest with U.combine to same effect.

polytypic commented 7 years ago

(I just mentioned the partial lenses naming as it is similar (variations of the same operation for different use cases). It doesn't do the same thing.)

rikutiira commented 7 years ago

Ah alright. 👍

So, which option would be the best? 3 variants?

polytypic commented 7 years ago

Hmm... Thinking about this I realized that the lifted functions U.concat, U.append and U.prepend already do similar things. Consider:

U.seq(value,
      U.of,
      U.concat(['foo']))

Of course, lifted functions have slightly different semantics (they give properties that skip identical values and perform deep lifting of the arguments) from plain Kefir combinators, but it might also make sense to provide flipped versions of those.

rikutiira commented 7 years ago

Been sick for a week so I haven't done anything about this yet.

That's not exactly the same, since it doesn't support passive properties. You could also use U.flatMapLatest to same effect as combine (with passive properties) but it ends up in a bit more complex code. Then again, library bloat makes API more complex.

Still unsure what would be best approach.