-
Notifications
You must be signed in to change notification settings - Fork 231
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
multicast seems to be unreliable #208
Comments
I’ve created a requirebin. This is the minimal example to reproduce the issue: var most = require('most');
var visibility = most.fromEvent('visibilitychange', document)
.map(function() { return !document.hidden; })
.startWith(!document.hidden)
.multicast(); // removing this will fix sampleWith
visibility
.forEach(function(value) {
document.body.innerHTML += 'first stream: ' + value + '<br>';
})
visibility
.sampleWith(most.periodic(1000))
.scan(function(acc, value) { return value; }, true) // removing this will also fix sampleWith
.forEach(function(value) {
document.body.innerHTML += 'sampled stream: ' + value + '<br>';
}) Looks like it’s a combination of
I hope this helps. 😕 |
Just observed this behaviour: changing tabs emits a |
Thanks for creating the requirebin. I'll take a look tonight. |
Started looking into this, and slowly whittling it down to a smaller test case. I'll keep you posted. |
Hey @maxhoffmann, I had a couple other things to deal with (and just released 0.18.4 w/a fix for another multicast-related issue), but I'll be back to investigating this tonight. |
Sure, don’t worry. There are very few open source projects that are as well maintained as this one. 😊 Thanks a lot for investigating! |
Ok, here's a "simplified" example that reproduces the issue. It's not really a lot simpler (it looks more complicated due to my adding var most = require('../../most');
var visibility = most.of(Date.now())
.concat(most.never())
.multicast(); // removing this will fix sampleWith
visibility
.forEach(function(value) {
console.log('1', value);
})
.then(function(x) { console.log('end 1', x); })
.catch(function(e) { console.error(e); })
most.combine((x, y) => x, visibility, most.periodic(1000))
.scan(function(acc, value) { return value; }, Date.now()) // removing this will also fix sampleWith
.forEach(function(value) {
console.log('2', value);
})
.then(function(x) { console.log('end 2', x); })
.catch(function(e) { console.error(e); }) So, baby steps :) |
I think I’m having the same problem today in a different place. Did you make any progress? |
Actually the issue I had today was not related to |
Hey @maxhoffmann, no worries. Glad you figured this recent one out. Sorry there hasn't been another update here. Other things have been stealing my attention, but I'll try to make progress this week. |
I think I've narrowed this down a bit further to an interaction between |
Awesome! Thanks for narrowing it down further. :) |
Here's another interesting clue. Rearranging the code in the simplified example above causes the problem to go away. This code is "doing" the same thing, it's just been reordered: var visibility = most.of(Date.now())
.concat(most.never())
.multicast();
most.combine((x, y) => x, visibility, most.periodic(1000))
.scan(function(acc, value) { return value; }, Date.now())
.forEach(function(value) {
console.log('2', value);
})
.then(function(x) { console.log('end 2', x); })
.catch(function(e) { console.error(e); })
visibility
.forEach(function(value) {
console.log('1', value);
})
.then(function(x) { console.log('end 1', x); })
.catch(function(e) { console.error(e); }) |
I just pushed the fix-208 branch, with an initial proof-of-concept fix. It works in the simplified example above. Can you try it out and let me know if it works for you? I'm not convinced yet that it's the right/best solution (I made some notes here, if want more gory details!), so I want to spend a little more time thinking about it and play with a few alternatives. Either way, I think we're really close to a resolution! Whew! |
Awesome! I’ll try the fixed branch next week. :) Thanks a lot! |
Hey @maxhoffmann, have you had a chance to try the fix-208 branch? |
Hey @briancavalier. No sorry not yet. I’ll give it a try tomorrow and let you know if it has fixed the problem. |
Thanks! |
Hey @briancavalier! Sorry for checking this a day later. I just tried the branch and Would be happy if this gets merged soon. :) Thanks for your work! |
That's great news, @maxhoffmann. I'll work on getting this into 0.19.1 by monday. |
@maxhoffmann 0.19.2 just landed. Thank you again for your help and patience on this issue! |
@briancavalier I have to thank you! Thanks for your time and work. Happy this could be resolved. :) |
I have this stream which stops emitting events to all subscribers (some still get the event) when I add
multicast
:I’m currently trying to reproduce a minimal test case for this. Interestingly adding
forEach
to the stream that stops receiving events makes it work again (withmulticast
).The text was updated successfully, but these errors were encountered: