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

Join parallel listeners to one #28

Closed
wants to merge 4 commits into from
Closed

Join parallel listeners to one #28

wants to merge 4 commits into from

Conversation

bripkens
Copy link
Contributor

@bripkens bripkens commented Aug 1, 2014

Add Reflux.all to track the status of multiple listenables.
All listenables must have emitted at least one.

Fixes #27

Add Reflux.all to track the status of multiple listenables.
All listenables must have emitted at least one.

Fixes #27
@bripkens
Copy link
Contributor Author

bripkens commented Aug 1, 2014

@spoike I will extend the README once I got confirmation that the proposed changes are okay.

@spoike
Copy link
Member

spoike commented Aug 1, 2014

We should probably have a test case that goes like this as well:

action1('a');
action3('c');
action1('x'); // should this argument be discarded or replaced?
action2('b');

The question remains is if we should discard the second call with action1 or replace it with the latest call.

We could decide the simplest solution to be the default behaviour and the other settable through an option?

@spoike
Copy link
Member

spoike commented Aug 1, 2014

Could also throw an error if this is an undesired behavior for the user, i.e. have that as an option for the user developer to provide a fail-fast way to indicate an action is invoked or a store is triggering itself out of intended order.

@bripkens
Copy link
Contributor Author

bripkens commented Aug 1, 2014

The question remains is if we should discard the second call with action1 or replace it with the latest call.

I went with the last call as I believe that you should only be interested in the last state. At least I cannot come up with a use case where you would want to observe a group of listenables and read all the states. This might also be out-of-scope for Reflux, no?

We could decide the simplest solution to be the default behaviour and the other settable through an option?

This may be a valid extension to this, especially the rejection when one action is called multiple times.

@spoike
Copy link
Member

spoike commented Aug 1, 2014

The last state option is okay with me. We can handle the rejection error throwing idea in another PR.

@spoike spoike added this to the 0.1.6 milestone Aug 1, 2014
@bripkens
Copy link
Contributor Author

bripkens commented Aug 1, 2014

If might look something like this:

var combined = Reflux.all(Reflux.all.strategy.trackLastCall, action1, action2, action3);

Possible strategies:

trackLastCall

This is the current behavior and I think it should be the default. Given the
test case:

action1('a');
action3('c');
action1('x'); // should this argument be discarded or replaced?
action2('b');

the combined action would emit with the following arguments:

[
    ['x'],
    ['b'],
    ['c']
]

trackFirstCall

Given the test case:

action1('a');
action3('c');
action1('x'); // should this argument be discarded or replaced?
action2('b');

the combined action would emit with the following arguments:

[
    ['a'],
    ['b'],
    ['c']
]

trackAllCalls

Given the test case:

action1('a');
action3('c');
action1('x'); // should this argument be discarded or replaced?
action2('b');

the combined action would emit with the following arguments:

[
    [['a'], ['x']],
    [['b']],
    [['c']]
]

ensureEqualNumberOfCalls

Given the test case:

action1('a');
action3('c');
action1('x'); // should this argument be discarded or replaced?
action2('b');

the combined action would not emit, but throw an exception saying that action1 was called twice.

@bripkens
Copy link
Contributor Author

bripkens commented Aug 1, 2014

Now that I am seeing this, I would say that we should do this in separate PR 😃

@spoike
Copy link
Member

spoike commented Aug 1, 2014

I'm okay with this PR as it is. Though I'll be waiting with merging things together for a 0.1.6 until we have circular dependency tracking. Lets take the tracking strategies in another PR...

@bripkens
Copy link
Contributor Author

bripkens commented Aug 1, 2014

Okay. I will add some documentation and the test case you mentioned.

@bripkens
Copy link
Contributor Author

bripkens commented Aug 1, 2014

@spoike Please check the README. I listed a few differences to waitFor() and it could certainly use a review :)

@bripkens
Copy link
Contributor Author

bripkens commented Aug 1, 2014

The comparison between Flux's waitFor() and our approach will need it's own heading, so it'll be easier for Flux users to know why we did it this way. Mostly because we can chain stores together in sequence and now also listen in parallel, which is an improvement over Flux. It is basically more composable (which is a key point we need to add at the introduction of the README).

Just added an h4 now even though I am not quite happy with this. It would be great if we had a top level section that would describe the difference between Flux and Reflux. Maybe we create such a section once #26, #27 and #28 have landed in master.

@spoike
Copy link
Member

spoike commented Aug 2, 2014

@bripkens I agree with having a top level section explaining the major differences in this regard. I will most likely edit the README later on once I get the ghpages up and running for the project. This feature is an important key point I believe. I hope users of Reflux will agree on this as well.

spoike added a commit that referenced this pull request Aug 5, 2014
@spoike
Copy link
Member

spoike commented Aug 5, 2014

Is merged into #34

@spoike spoike closed this Aug 5, 2014
@krawaller krawaller mentioned this pull request Oct 2, 2014
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

Successfully merging this pull request may close these issues.

Joining parallel listeners to one
2 participants