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

Curried Kefir.combine #5

Open
rikutiira opened this issue Jun 11, 2017 · 6 comments
Open

Curried Kefir.combine #5

rikutiira opened this issue Jun 11, 2017 · 6 comments

Comments

@rikutiira
Copy link
Member

rikutiira commented Jun 11, 2017

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

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

rikutiira commented Jun 12, 2017

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

(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
Copy link
Member Author

Ah alright. 👍

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

@polytypic
Copy link
Member

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

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.

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

No branches or pull requests

2 participants