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

What to do with Bus().toProperty()? #729

Open
raimohanska opened this issue Dec 17, 2018 · 18 comments
Open

What to do with Bus().toProperty()? #729

raimohanska opened this issue Dec 17, 2018 · 18 comments

Comments

@raimohanska
Copy link
Contributor

I've read (and answered to) numerous complaints that code involving bus.toProperty() "does not work". And I totally agree that it's counterintuitive that something like this (from Bacon FAQ) does not work:

var bus = new Bacon.Bus;
var property = bus.toProperty('FIRST');
bus.push('SECOND');
property.onValue(function(val) {
  console.log(val);
});

You'd like the console to log 'SECOND' but it logs 'FIRST' instead.

What's happening here is that your Property won't get updated when there are no listeners. Before you add an actual listener using onValue, the Property is not active; it's not listening to the underlying Bus. It ignores input until someone's interested. So it all boils down to the "laziness" of all Bacon streams and properties. This is a key feature too: the streams automatically "plug in" to their sources when a subscriber is added, and "unplug" when there are no more subscribers.

A simple patch for this would be to have Bus::toProperty() always add a dummy subscriber to the resulting Property. Then, again, it would fail in setups like this:

bus.map(x => x*2).toProperty();

... so it's not such a great patch after all. Or is it? Because if you flipped it like

bus.toProperty().map(x => x*2);

it would be good again!

The underlying problem is that the subscriber-count-based mechanism for change propagation in Bacon.js doesn't serve you well in cases when you create a stateful Property on top of a stateless EventStream unless you always have at least one subscriber on the Property.

One option I'm half-seriously considering is to have Properties throw an error in cases where you re-subscribe after the last subscriber has been removed. This is probably always a bug in application code, because it can lead to propagating outdated values.

Thoughts?

@semmel
Copy link
Member

semmel commented Dec 17, 2018

One option ... is to have Properties throw an error in cases where you re-subscribe after the last subscriber has been removed.

Maybe this is not so bad: Letting the application actively decide for lazyness or eagerness.

Let's follow this path for a moment:
How would I implement such a common pattern, like making a shared resource available as Property to clients which come and go (i.e are created and destroyed)? I guess only by making the property artificially "eager" ("hot") with a dummy subscriber as suggested.
But then we'd loose that lazyness: Even when there are no "real" observers the whole expensive subscription and mapping machinery of the property is kept running.
If these useless computations are a problem for the application it could manage property lifetime itself like this

// pseudo-code
var prop = observers.length
.map(l => l > 0)
.skipDuplicates()
.flatMapLatest( areAny => areAny ? createProperty(...) : Bacon.never() );

observers
.onValue(o => prop.onValue(o));

On the other hand, when keeping the property eagerly up-to-date is no problem for the application, then it's just the "eager-making" the application is responsible for.

In the other case, if one wants to retain lazyness and be able to re-subscribe to property - I have no idea. On re-subscription one would have to travel backwards in time and replay all events which lead to the current value. Any .reduce, .scan etc. would make this impossible anyway.

🤔

@semmel
Copy link
Member

semmel commented Dec 18, 2018

And I totally agree that it's counterintuitive that something like this (from Bacon FAQ) does not work:
...

Why is it counterintuitive?
Because the documentation says: "A property has the concept of "current value"." ? The Readme explains it a bit more: "So things that change and have a current state are Properties, while things that consist of discrete events are EventStreams. You could think mouse clicks as an EventStream and mouse position as a Property."

Then why not make all Properties eager? This would acknowledge that a Property ("thing with a state") has to be kept up-to-date regardless of its observers until it is garbage-collected by the JS runtime.

I think the current description of Properties does not tell the whole story anyway. Should it not rather be (emphasised text added, striked-out text goes away in case of all-eager properties):

Bacon.Property a reactive property. Has the concept of "current value". .... Note: depending on how a Property is created, it may or may not have an initial value. The current value is the initial value, or the most recent value until there are no more observers. The current value is the first value published to a new observer and stays as its last (only) value after the stream has ended.

In the proposal

to have Bus::toProperty() always add a dummy subscriber to the resulting Property.

would var property = bus.toProperty('FIRST'); kind of mutate bus? I.e. would I need to think about the internal state (eagerness) of bus when I do bus.push('SECOND')? Then I'd rather prefer another type of immutable "Property-Bus", or otherwise reasoning about bus.push would require knowledge about the timings of all bus.toProperty() calls?

@raimohanska
Copy link
Contributor Author

A lot of questions in a single comment by friend!

Making all properties eager would, if I understand your suggestion correctly, prevent them from being garbage-collected because they are referenced by the underlying streams keeping them up to date. Unless, of course, we somehow were able to use weak references for the supply chain. Unlikely so.

I don't really believe my patchy little suggestion above would be a good idea. Would make one case work better but still leave room for error in more complex setups. Let's just forget about it.

@semmel
Copy link
Member

semmel commented Jan 31, 2019

Considering the Lazyness of Properties an observation which is not clear to me:

Right now a subscription to an observable turns it's dependency tree of combined observables eager (i.e. stateful). With the direction from bottom to top.

/* p1   p2  ^
    \  /    |  eagerness travels upwards
   p1orp2   |
*/
var p1 = Bacon.repeatedly(11000, [true, false]).toProperty(false);
var p2 = Bacon.repeatedly(15000, [true, false]).toProperty(false);
var p1orp2 = p1.or(p2);
// this makes p1orp2 and its dependencies p1 and p2 stateful
p1orp2.onValue(x => x); // false, true, true, true, false, true, false, true ...

However, does the eagerness of the tree also travel down into the combined trunk? I mean, does the subscribing to p1 and p2 make p1orp2 eager too?
Without looking at the source code I find this is often the case. Although I would not expect that.
However I find the surprising behaviour in this particular example:

// p1, p2 and p1orp2 see above
var p1, p2, p1orp2 = ...
// just make the p1 and p2 dependencies eager
p1.or(p2).log("p1orp2");
Bacon.mergeAll(
	Bacon.later(7000, "A"), 
	Bacon.later(12000, "B"), 
	Bacon.later(19000, "C"), 
	Bacon.later(35000, "D")
).flatMapConcat(
	x => Bacon.once(x).holdWhen(p1.or(p2)).delay(500)
).log();
// p1orp2 false
// A  <-- correct because no holding
// p1orp2 true, p1orp2 true, p1orp2 true <-- correct no emissions
// p1orp2 false B C <-- correct B is released now and C passes because no holding
// p1orp2 true  D  <--- WRONG D must not pass while p1 || p2 is true
// <end>

With bottom up eagerness the example works like expected. The code is nearly identical but one has to introduce a seemingly useless variable and not forget to make it eager explicitly.

// p1, p2 and p1orp2 see above
var p1, p2, p1orp2 = ...
// make the whole tree eager;
p1orp2.log("p1orp2"); 
Bacon.mergeAll(
	Bacon.later(7000, "A"), 
	Bacon.later(12000, "B"), 
	Bacon.later(19000, "C"), 
	Bacon.later(35000, "D")
).flatMapConcat(
	x => Bacon.once(x).holdWhen(p1orp2).delay(500)
).log();
// p1orp2 false
// A  <-- correct because no holding
// p1orp2 true, p1orp2 true, p1orp2 true <-- correct no emissions
// p1orp2 false B C <-- correct because no holding anymore
// p1orp2 true
// p1orp2 false D <-- correct because no holding
//<end>

So if eagerness would propagate also from top to bottom one could write .holdWhen(p1.or(p2)) or .holdWhen(p1.and(p2)) or whatever combinator without the need to manage an extra instance of the combined observable.

Anyway I don't understand why sometimes a combined property seems to "work a little" even though
there is no "dummy" subscriber. I.e I wonder why the second example above not breaks down right away, but after some iterations? Why does .holdWhen(p1.or(p2)) let pass D when p1 is true ?

@steve-taylor
Copy link

var bus = new Bacon.Bus;
var property = bus.toProperty('FIRST');
bus.push('SECOND');
property.onValue(function(val) {
  console.log(val);
});

To me, it makes perfect sense that 'SECOND' will be missed. It’s maybe unintuitive at first, but once you realize how observables work, anything else would be unintuitive.

One option I'm half-seriously considering is to have Properties throw an error in cases where you re-subscribe after the last subscriber has been removed. This is probably always a bug in application code, because it can lead to propagating outdated values.

I prefer and rely on Property retaining its state after the last subscriber unsubscribes. I understand how it can feel wrong, but I think throwing an error is a bit extreme.

How about simply dropping its state if it was obtained lazily? That's a can of worms, though. If a Property has had many events and its initial state was created at initialization (e.g. constant(1)), after all subscribers have unsubscribed, does the next subscriber get the initial value of 1 or the latest value? I think it should get the initial value.

@semmel
Copy link
Member

semmel commented Aug 18, 2020

To me, it makes perfect sense that 'SECOND' will be missed.

I still don't think that makes sense. How can the first item in a multicast property stream depend on the temporal order of subscriptions from other consumers in entirely different modules of the application? The current subscriber has no idea when other subscribers hook into the property stream.

How would you model such a trivial thing as a multicast property of the bowser's window size in baconjs?

var windowSize = 
  Bacon.fromEvent(window, 'resize')
  .map(evt => ({width: evt.target.innerWidth, height: evt.target.innerHeight}))
  .toProperty({width: window.innerWidth, height: innerHeight});

var unsubscribe = windowSize.onValue(console.log.bind(console));

In temporal subscription gaps to windowSize the resize event handler is removed and the property is not updated. Resubscription to windowSize will definitely produce a faulty first value.

In my applications I carefully pay attention to manage a continuous lifetime of the such multicast properties. I think I am all-in for throwing an exception on resubscription to a property.

@steve-taylor
Copy link

steve-taylor commented Aug 18, 2020

I still don't think that makes sense.

It makes sense to me because bus has no subscribers when bus.push('SECOND'); is called.

How can the first item in a multicast property stream depend on the temporal order of subscriptions from other consumers in entirely different modules of the application? The current subscriber has no idea when other subscribers hook into the property stream.

(Emphasis mine)

Ah, now we're talking about something else – values being pushed to bus when, through static analysis, we don’t know whether or not bus has a subscription. Which segues into your example:

var windowSize = 
  Bacon.fromEvent(window, 'resize')
  .map(evt => ({width: evt.target.innerWidth, height: evt.target.innerHeight}))
  .toProperty({width: window.innerWidth, height: innerHeight});

var unsubscribe = windowSize.onValue(console.log.bind(console));

I think I am all-in for throwing an exception on resubscription to a property.

You should also be all-in for throwing an exception on the very first subscription to windowSize. You correctly identified the issue of there potentially being a large temporal gap between the subscription count increasing from 0 to 1 after it had previous subscribers, thereby making the Property’s last event stale. However, you have missed the fact that the very first subscriber to windowSize may subscribe at any time, not necessarily immediately after windowSize has been created. Therefore the value passed to .toProperty is also potentially stale.

This illustrates the danger of .toProperty(initialValue) existing at all.

Here’s my proposal:

  • When the last subscriber to a Property unsubscribes, clear its state, because it’s stale.
  • Remove .toProperty(initialValue) because initialValue can be stale by the time the first subscriber subscribes.
  • Add .toProperty(getInitialValue) to get a fresh initial value for the first subscriber.

Using the above proposal, your Property could be safely rewritten as follows:

var windowSize = 
  Bacon.fromEvent(window, 'resize')
  .map(evt => ({width: evt.target.innerWidth, height: evt.target.innerHeight}))
  .toProperty(() => ({width: window.innerWidth, height: innerHeight}));

When the subscription count increases from 0 to 1, the callback passed to toProperty will be called and its result will seed the Property with an initial value for subscribers. This will happen whether or not the Property has previously had subscribers.

Alternatively, if there's a reasonably concise and expressive way to achieve this with current Bacon constructs, then .toProperty(getInitialValue) may not be needed.

@semmel
Copy link
Member

semmel commented Aug 18, 2020

I still don't think that makes sense.

It makes sense to me because bus has no subscribers when bus.push('SECOND'); is called.

That's the technical explanation of the behaviour but not the explanation why in the context of reactive streams one should expect such a behaviour.
Such a property stream is mutated from "live" to "lazy" at – if we're async – nondeterministic points in time, thus it is not const and not referential transparent. This makes it hard to use correctly.

Add .toProperty(getInitialValue) to get a fresh initial value for the first subscriber.

Yes, making .toProperty lazy could solve some problems. It's a good idea, but sometimes it's not possible to pull the current value into the property stream, for instance if it's derived from a WebSocket connection where the values are pushed into the stream.

Alternatively, if there's a reasonably concise and expressive way to achieve this with current Bacon constructs, then .toProperty(getInitialValue) may not be needed.

windowSize = Bacon.fromBinder(sink => {
  sink({width: window.innerWidth, height: innerHeight});
  window.addEventListener('resize', sink); // I left out mapping from ResizeEvent to {width: Number, height: Number}
  return () => {
    window.removeEventListener(sink);
  };
})
.toProperty();

Should do the job but it is not as pretty as .toProperty(getInitialValue);.

Edit:
I think in general getInitialValue should return a promise.

@steve-taylor
Copy link

Should do the job?

I thought “reasonably concise and expressive” implied “Don’t use Bacon.binder” 😆

Yes, making .toProperty lazy could solve some problems. It's a good idea, but sometimes it's not possible to pull the current value into the property stream, for instance if it's derived from a WebSocket connection where the values are pushed into the stream.

The idea is to make .toProperty get its value just in time, not lazily. This is so the initial value is sampled from the current state of some resource exactly at the point in time that the first subscriber subscribes, while guaranteeing that the subscriber receives its first event synchronously.

I think in general getInitialValue should return a promise.

I wouldn’t want to have to provide a Promise to .toProperty` when I know I can sample the initial state synchronously. e.g. this is too much:

var windowSize = 
  Bacon.fromEvent(window, 'resize')
  .map(evt => ({width: evt.target.innerWidth, height: evt.target.innerHeight}))
  .toProperty(() => Promise.resolve({width: window.innerWidth, height: innerHeight}));

Also, I wouldn’t want .toProperty to magically detect and unpack Promises to give me the flexibility of returning a value or a Promise. It would be too much magic and I don’t think there’s a precedent in Bacon for dealing with Promises in this way. This is probably what Bacon.fromBinder is for.

@semmel
Copy link
Member

semmel commented Aug 18, 2020

The idea is to make .toProperty get its value just in time, not lazily.

Lazy evaluation means that the value is computed when needed and not at the time when the property is defined. .toProperty(initialValue) computes the initial value at definition time while toProperty(getInitialValue) executes getInitialValue every time the property is re-subscribed.

I wouldn’t want to have to provide a Promise to .toProperty` when I know I can sample the initial state synchronously.

There are plenty of examples in the browser or node.js APIs where you cannot pull a value synchronously (e.g. navigator.mediaDevices.enumerateDevices(), retrieving data from IndexedDb, fetch, navigator.geolocation.getCurrentPosition, ...). How would you implement for example the getInitialValue function for a GeolocationPosition property?

Anyway – returning a promise or not – it does not solve the problem where the values are pushed into the property from elsewhere (e.g. WebSocket) and the current value cannot be pulled in. Contrary to intuition values are lost in a non-plausible way. Edit: Unless properties are made eager at definition time and maybe get a dispose() method for deterministic clean-up.

@steve-taylor
Copy link

steve-taylor commented Aug 19, 2020

How would you implement for example the getInitialValue function for a GeolocationPosition property?

Think about it for a moment. If the part of the stream before .toProperty is emitting events as they occur, then .toProperty(somePromise) is going to emit a duplicate event.

For example:

const windowSize = Bacon
  .fromEvent(window, 'resize')
  .map(evt => ({width: evt.target.innerWidth, height: evt.target.innerHeight}))
  .toProperty(() => new Promise(resolve => (
    const listener = evt => {
      // Duplicate event - will also be emitted by fromEvent
      resolve({width: evt.target.innerWidth, height: evt.target.innerHeight})
      window.removeEventListener('resize', listener)
    }
    window.addEventListener('resize', listener)
  ))

This is a bit contrived, but I think it illustrates the point.

@semmel
Copy link
Member

semmel commented Aug 19, 2020

What I meant was something like

...
.toProperty(() => new Promise(resolve => navigator.geolocation.getCurrentPosition(resolve)));

@steve-taylor
Copy link

Presumably, this is the part before .toProperty:

const position$ = Bacon
    .fromBinder(sink => {
        const handle = navigator.geolocation.watchPosition(sink)
        return () => Geolocation.clearWatch(handle)
    })

What’s the point of asynchronously resolving the first event when we’re going to do it anyway via watchPosition? Can’t we simply add .toProperty() and be done with it? If we’re resolving the initial value asynchronously, it means it’s not available. It will be available later, so why not just subscribe to the underlying stream and get the initial value later anyway?

@semmel
Copy link
Member

semmel commented Aug 19, 2020

Yes, if we use Bacon.fromBinder we might not need a promise-returning function in .toProperty().
I thought your point was not to use Bacon.fromBinder. 😉
I made that Geolocation example just to demonstrate that getting initial values is sometimes asynchronous, opposite to my windowSize example.

Imagine one had written a Bacon.fromGeolocationChangesEventStream(). Then you'd perhaps make a property of it by

Bacon.fromGeolocationChangesEventStream()
.toPropery(() => new Promise(resolve => navigator.geolocation.getCurrentPosition(resolve)));

in order to have the current position asap and not wait for movement to get a fix on the location. (Just another made-up example, please take it with caution)

@steve-taylor
Copy link

Imagine one had written a Bacon.fromGeolocationChangesEventStream(). Then you'd perhaps make a property of it by

Bacon.fromGeolocationChangesEventStream()
.toPropery(() => new Promise(resolve => navigator.geolocation.getCurrentPosition(resolve)));

The first subscriber to Bacon.fromGeolocationChangesEventStream() would presumably read and emit the initial state in the setup (on first subscription), thereby rendering .toProperty(promiseThatGetsLocation) redundant.

Suppose we had Bacon.fromTextInputValue(element) and it returns an EventStream. I don’t know about you, but I’d expect the first subscriber to immediately (although slightly asynchronously, because it’s an EventStream) receive an event containing the initial value. Its utility is compromised if the initial value of the input has to be retrieved manually.

@semmel
Copy link
Member

semmel commented Aug 19, 2020

Because it's fun, another example without Bacon.fromBinder

var clipboardText = 
  Bacon.fromEvent(document, 'paste')
  .flatMapLatest(() => Bacon.fromPromise(
    navigator.clipboard.readText()
  ))
  .toProperty(() => navigator.clipboard.readText()); // <-- returns a promise

See Asynchronous Clipboard API

@steve-taylor
Copy link

If Bacon.fromClipboard existed, it would probably do the initial readText() for the first subscriber on setup 😛

@raimohanska
Copy link
Contributor Author

Hi guys! I wrote something on property reactivation on #770. 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

No branches or pull requests

3 participants