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

Demonstrate possible issue #37

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Demonstrate possible issue #37

wants to merge 5 commits into from

Conversation

jkopanski
Copy link

Hello and thank you for awesome library!

Recently I came across a use case that I thought was good fit for hold. Unfortunately it didn't work as expected. Before opening up the issue I thought that maybe I'll clarify what I do and what outcome I expect with a minimal test case.

I maintain an collection of streams (array in the example). Elements will come and go depending on the user interactions. That array is later combined to produce final result. I thought that each combine call is like creating a now observer for the held stream, however previous value isn't provided.

Is this how hold is suppose to work? Or did I actually found some issue here?

@briancavalier
Copy link
Member

Hi @jkopanski. Thanks for describing your use case and for taking the time to create a test case to show it. I'm not sure I fully understood, though.

Do you think you could try to find a simplest-possible example of what you think the issue is?

Thanks!

@raveclassic
Copy link
Contributor

raveclassic commented Aug 1, 2019

I've just ran into exactly the same issue. However my case is slightly different.
It turns out that hold only delivers the last value if and only if it has at least one active subscriber. This suits the general most idea about a single run but it breaks on edge cases with multiple runs/disposals.
STR:

  1. hold some hot source
  2. run the held source
  3. emit some value in the source (the value will propagate to the held source)
  4. dispose the held source (this clears the sinks properties)
  5. run the held source once again
  6. the last value should propagate to the subscriber asap but it is not because of the sinks length check

@raveclassic
Copy link
Contributor

raveclassic commented Aug 1, 2019

Here're some marbles:

A:  ---a------b--
H:  ---a------b--
S1: -^---!
R1: ---a-!
S2: -------^-----
R2: -------?--b--

Legend: A - hot source, H - held source, S1 - first subscription (^ - run, ! - dispose), R1 - events in the first subscriber, S2 - second subscription, R2 - events in the second subscriber.

Here you can see that 2nd subscriber runs the stream after the first disposal and it should receive both events a and b but it does not because on the second run hold checks if there is at least one active subscriber before scheduling a flush.

@jkopanski
Copy link
Author

Hey @briancavalier, thanks for your patience.

I'll try and possibly simplify testcase but I'm bit swamped at work.
However @raveclassic summarized the issue nicely. It seems that hold works only if there is at least one active subscriber. I've opened this draft to figure out is this by design or in fact some possible issue.

For now we copied hold and perform only a value check before scheduling flush.

@raveclassic
Copy link
Contributor

raveclassic commented Aug 2, 2019

For now we copied hold

Same for me :)

@jkopanski
Copy link
Author

However that breaks one of original test cases, so I am not sure if this comibnator was intended to be used that way

@raveclassic
Copy link
Contributor

@jkopanski This happens because verifyHold holds a cold source:

const s = hold(mergeArray([at(0, 0), at(10, 1), at(20, 2)]))

Then first run is made by p0's collect (Note that the source is diposed by take(1)):

hold/test/index-test.ts

Lines 32 to 33 in d57c408

const p0 = collect(take(1, s), scheduler)
.then(events => eq([0], events))

The second run is then made by p1's collect 5ms after:

hold/test/index-test.ts

Lines 54 to 56 in d57c408

delay(5, {
run () {
resolve(collect(stream, scheduler))

hold sends the last remembered value from the first run "0" and then reruns the source resulting in the [0, 0, 1, 2].

Here're some marbles describing the failing test:

timeline   : -   -   -   -   -   -   -   -   -   -   -
hold       : 0   1   (2!)
take(1)    : ^  
           : (0!)     
delay(5)   : -   -   -   -   ^
           : -   -   -   -   (00)1   (2!)
                              ||                
                             /  \
                         hold    rerunning source 

I'm not exactly sure what should be the correct behavior for holding cold sources but I think this (which breaks the test) is definitely more intuitive for me and it reflects the description of the package: Deliver the most recently seen event to new observers.

@briancavalier What do you think?

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.

3 participants