Make Kefir.combine() also accept sources as objects

[rpominov]

Currently combine accepts source observables in arrays, but nothing stops us from also supporting objects as containers for sources.

For instance:

const sumStream = Kefir.combine({a: aStream, b: bStream}, ({a, b}) => a + b)

This basically will make combine support named parameters, which is better than parameters as a list.

Also this will work:

Kefir.combine({a: aStream, b: bStream}).log()
// <value> {a: 1, b: 2}

Note: not proposing supporting nested objects, or simple values beside observables, as Bacon. combineTemplate() does. Only shallow "maps" of {[string]: Observable}.

If someone wants to do this, PR is very welcomed!

[niahoo]

Hi !

I'm interested into this. What do you think about passive sources ?

[rpominov]

Hey, @niahoo! I think passive sources should work as well e.g., we should be able to do Kefir.combine({a: aStream}, {b: bStream}, ({a, b}) => a + b)

[mAAdhaTTah]

I'm looking at implementing this, as this would be really useful for something I'm working on. This one is easy enough:

Kefir.combine({a: aStream, b: bStream}).log()
// <value> {a: 1, b: 2}

How do we output the instance method?

aStream.combine({b: bStream}).log()
// <value> {[key?]: 1, b: 2}

[rpominov]

I think we should add support only to the static method.

[rpominov]

Thanks that you're looking into it, btw!

[iofjuupasli]

https://github.com/iofjuupasli/kefir-combine-object

It's far from perfect (Ramda dependency), but if you need object combine right now, this can be helpful

[32bitkid]

I've pulled out a wrapper that I often use; does it need to alter the existing combine() or should it be its own method? I took the safer road with ce68e16; combineProps() (name influenced by .props() in bluebird which does something similar) is just syntatic sugar on top of combine().

Was thinking about rolling the implemenation together, but then was kind of weirded out about mixed signatures:

Kefir.combine({a: aStream, b: bStream}, [cStream, dStream], (???) => {})

[rpominov]

I'm on the fence. We certainly should not support combining objects with arrays, so if we overload combine and someone calls it with an object and array it should just throw. But maybe having separate method is also a good idea.

Really not sure what is the best call here, if you're about to make a PR just do what feels right for you 😄

One more thing: I think this method should not wrap existing combine, should be implemented similarly to combine itself, either as part of combine implementation or as a separate method with similar code.

[32bitkid]

Sounds good. I'll marinate on it a bit and then make a proper pr

On Oct 20, 2016, at 4:03 AM, Roman Pominov notifications@github.com wrote:

I'm on the fence. We certainly should not support combining objects with arrays, so if we overload combine and someone calls it with an object and array it should just throw. But maybe having separate method is also a good idea.

Really not sure what is the best call here, if you're about to make a PR just do what feels right for you 😄

One more thing: I think this method should not wrap existing combine, should be implemented similarly to combine itself, either as part of combine implementation or as separate method with similar code.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

[mAAdhaTTah]

This landed already so we can close this one.