[Discuss] expose BahviorSubject instead of Observable

[streamich]

TLDR; Add .getValue() method to observables exposed from core services which have concept of current value.

---

Problem

Often platform APIs expose observables to plugins, which is great for changing data. However, sometimes those observables have current value which they emit immediately on subscription. Often devs are interested just in the current value, however there is no convenient way to fetch it, devs need to subscribe to the observable, take one value and unsubscribe.

To get the current app ID, right now devs need to do somethings like this:

core.application.currentAppId$
  .pipe(take(1))
  .subscribe((appId: string | undefined) => (this.currentAppId = appId));

Instead, it could be something like this:

this.currentAppId = core.application.currentAppId$.getValue();

In this case it would even be possible to put it at class level:

class Foo {
  private readonly currentAppId = this.application.currentAppId$.getValue();
}

Solution

Expose observables that have concept of current value as "behavior subject". It does not need to be a complete BehaviorSubject, it just needs to have .getValue() method. It could be called ReadonlyBehaviorSubject.

type ReadonlyBehaviorSubject<T> = Observable<T> & Pick<BehaviorSubject<T>, 'getValue'>;

Where?

Above was an example of Application service, but this applies to a number of other places where observable with current value is exposed, here are some such observables in Chrome service:

[elasticmachine]

Pinging @elastic/kibana-platform (Team:Platform)

[mshustov]

probably will be done in #53268 as part of Provide a way to access config synchronously (?)

[streamich]

probably will be done in #53268 as part of Provide a way to access config synchronously (?)

@restrry that would be great, however, my understanding is that "config" in that sentence refers to uiSettings service, but would be great to have this in all observables with current value in all services.

[rudolf]

I'm 👍 on implementing this for most of Core's observables where the observable emits the "latest value".

@restrry that would be great, however, my understanding is that "config" in that sentence refers to uiSettings service, but would be great to have this in all observables with current value in all services.

That config is referring to kibana.yml config values, but I think we can establish the pattern in that issue and then apply it to other parts of Core.

[rudolf]

The downside to this is that it's a backdoor to go from reactive code to imperative. Sometimes it's necessary or very awkward not to do it, I think particularly when you need to read a config value from kibana.yml to call a Core API from setup.

If you have a long-lived class your example would be better left as an subscribe to make sure currentAppId stays up to date:

core.application.currentAppId$
  .subscribe((appId: string | undefined) => (this.currentAppId = appId));

But something like

const shortLivedVariable = await core.application.currentAppId$
  .pipe(take(1)).toPromise();

Would benefit from using a toValue().

[streamich]

If you have a long-lived class your example would be better left as an subscribe to make sure currentAppId stays up to date:

core.application.currentAppId$
  .subscribe((appId: string | undefined) => (this.currentAppId = appId));

You forgot to unsubscribe in that example. 🤓

I would argue in a long-lived class, if you are not using any or RxJS programming patterns, you are still better off using .getValue() (or .toValue()), either directly

this.core.application.currentAppId$.getValue()

or wrapping it into a local helper

private readonly currentAppId = () => this.core.application.currentAppId$.getValue();

reason being you don't need to unsubscribe and you don't need to know RxJS (and less code).

[streamich]

Another issue with having to .subscribe() is that a long-living class might not have a life-cycle where to unsubscribe, like here: #62865 (comment)

[mattkime]

I've seen need for this as well, particularly when setting the initial state in a react component. Imperative code works for this in ways that reactive code does not mostly due to typescript.

[pgayvallet]

Overall I understand the need and I'm +1 on that, even if imho we should be cautious before implementing it everywhere. There may be places where we want to avoid synchronous obs value getters, simply to encourage the reactive approach instead (or/and because using non-reactive approach can be dangerous).

Expose observables that have concept of current value as "behavior subject". It does not need to be a complete BehaviorSubject, it just needs to have .getValue() method. It could be called ReadonlyBehaviorSubject.

Taking a random example from core:

kibana/src/core/public/application/application_service.tsx

Lines 277 to 281 in 4330865

	currentAppId$: this.currentAppId$.pipe(
	filter(appId => appId !== undefined),
	distinctUntilChanged(),
	takeUntil(this.stop$)
	),

this.currentAppId$ is a subject, however .pipe only returns an observable.

Also not all our observable are behaviorSubjects, meaning that some can have NO last emitted value. So the actual type would be (ihmo we should not name if from BehaviorSubject as it should also work with plain Observable):

// yea the name suck. Please find something better
type ObservableWithGetValue<T> = Observable<T> & {
   getLastValue(): T | undefined;
};

We would need to introduce a wrapper, something like

// very naive implem. Please break it. Maybe we will need our own `Observable` subclass instead.
// i.e what should be the `lastValue` once the source obs is completed?
function addGetLastValue<T>(obs: Observable<T>): ObservableWithGetValue<T> {
  let lastValue: T | undefined;
  const piped = obs.pipe(
    tap(value => {
      lastValue = value;
    })
  );
  return Object.assign(piped, {
    getLastValue: () => lastValue,
  });
}

and use it like

currentAppId$: addGetLastValue(this.currentAppId$.pipe( 
   filter(appId => appId !== undefined), 
   distinctUntilChanged(), 
   takeUntil(this.stop$) 
 )), 

[streamich]

That currentAppId$ still has a current value, i.e it is effectively a "behavior subject", its current value can be accessed synchronously, even though it was converted to Observable.

I would suggest a simple function that converts Observable back to BehaviorSubject.

const observableToBehaviorSubject = <T>(currentValue: T, obs: Observable<T>): BehaviorSubject<T> => {
  const subj = new BehaviorSubject(currentValue);
  obs.subscribe(subj);
  return subj;
};

and then currentAppId$ can be

currentAppId$: observableToBehaviorSubject(
  this.currentAppId$.getValue(),
  this.currentAppId$.pipe( 
     filter(appId => appId !== undefined), 
     distinctUntilChanged(), 
     takeUntil(this.stop$) 
  )
), 

[pgayvallet]

That currentAppId$ still has a current value, i.e it is effectively a "behavior subject", its current value can be accessed synchronously, even though it was converted to Observable.

Yea it does. But as I already said, all our exposed observable are not necessarily chained from Subjects.

const observableToBehaviorSubject = (currentValue: T, obs: Observable): BehaviorSubject => {
const subj = new BehaviorSubject(currentValue);
obs.subscribe(subj);
return subj;
};

Would need other team member's opinion, but I personally really don't like the idea of returning concrete subjects from our APIs.

Also this doesn't work if you don't know the currentValue, meaning that you will not be able to use this with plain Observable.(and once again, we don't have only concrete Subjects)

[streamich]

Yea it does. But as I already said, all our exposed observable are not necessarily chained from Subjects.

This proposal is only for subjects. I'm not suggesting to do anything for plain observables.

... I personally really don't like the idea of returning concrete subjects from our APIs.

It does not need to be BehaviorSubject it just needs to have a way to access current value, as suggested in OP, it could be ReadonlyBehaviorSubject:

type ReadonlyBehaviorSubject<T> = Observable<T> & Pick<BehaviorSubject<T>, 'getValue'>;

Or it could have .toValue() method like suggested by @rudolf. Or it could have .getLastValue() like in your example.

Benefit of using .getValue() is that you can simply cast the object:

currentAppId$: ReadonlyBehaviorSubject<string | undefined> = ...

[pgayvallet]

This proposal is only for subjects. I'm not suggesting to do anything for plain observables.

As we are only returning Observable in our APIs, this suggestion means that the feature would/should only be available on some of the APIs depending on the implementation detail that we are, or not, using a Subject as the observable's source?

What about a plain Observable with a replay effect? it's not a Subject, but do (potentially) emit an initial value on subscription. Same thing with any cold observable.

To take a concrete example from the chrome service:

kibana/src/core/public/chrome/chrome_service.tsx

Lines 111 to 130 in 4330865

	this.isForceHidden$ = new BehaviorSubject(isEmbedded);
	const appHidden$ = merge(
	// For the isVisible$ logic, having no mounted app is equivalent to having a hidden app
	// in the sense that the chrome UI should not be displayed until a non-chromeless app is mounting or mounted
	of(true),
	application.currentAppId$.pipe(
	flatMap(appId =>
	application.applications$.pipe(
	map(applications => {
	return !!appId && applications.has(appId) && !!applications.get(appId)!.chromeless;
	})
	)
	)
	)
	);
	this.isVisible$ = combineLatest([appHidden$, this.isForceHidden$]).pipe(
	map(([appHidden, forceHidden]) => !appHidden && !forceHidden),
	takeUntil(this.stop$)
	);

kibana/src/core/public/chrome/chrome_service.tsx

Line 217 in 4330865

getIsVisible$: () => this.isVisible$,

Adding a sync accessor for the this.isVisible$ / getIsVisible$ observable would probably make as much sense as adding it for currentAppId$. However this is a 'complex' combination of other observables. (note that this obs do have an initial value)

Even If we accept the postulate that application.currentAppId$ would already have been transformed to a ReadonlyBehaviorSubject using observableToBehaviorSubject, how would you properly convert this.isVisible$ the same way with your suggested approach without duplicating the logic in the map(([appHidden, forceHidden]) => !appHidden && !forceHidden) block to get the initial value?

It does not need to be BehaviorSubject it just needs to have a way to access current value, as suggested in OP, it could be ReadonlyBehaviorSubject

With your given observableToBehaviorSubject implementation, even if the signature was hiding the actual implementation by having it's signature declaring a ReadonlyBehaviorSubject return, the actual, concrete object returned is a BehaviorSubject, meaning that the whole BehaviorSubject API would be accessible using force-cast.

[streamich]

What about a plain Observable with a replay effect?

It would be great if you would add sync accessors for those, too. For that few extra lines would need to be added on top of observableToBehaviorSubject. Or, it does not need to be observableToBehaviorSubject, it's just one example.

Adding a sync accessor for the this.isVisible$ [...] how would you properly convert this.isVisible$ ...

I'm not familiar with code there, but my initial guess would be that maybe this could work:

getIsVisible$: ReadonlyBehaviorSubject = () =>
  observableToBehaviorSubject(false, this.isVisible$);

With your given observableToBehaviorSubject implementation, even if the signature was hiding the actual implementation by having it's signature declaring a ReadonlyBehaviorSubject return, the actual, concrete object returned is a BehaviorSubject, meaning that the whole BehaviorSubject API would be accessible using force-cast.

I'm not sure I would call it "hiding", it is just casting. Curious, what is your point here? If you do .toObservable() the whole API is also accessible through force casting, e.g subj.toObservable().source.next(...). And whatever you do, most likely the API will be accessible through some trickery and force casting, because it is JavaScript.

Let's say you have:

class Animal {}
class Dog extends Animal {}

const dog = new Dog();

and method

function feedAnimal(anima: Animal) {}

would you do?

feedAnimal(dog);

or?

feedAnimal(dog.toAnimal());

[joshdover]

We introduced Observable-based APIs as a forcing function on consumers to be handle changing values. I believe introducing a sync access pattern will inevitably lead to bugs where consumers are not handling changing values and utilize stale data.

So if we were to introduce a sync pattern, we should do so carefully and only with APIs where having a stale value would not be potentially dangerous. Even then, I'm not sure the tradeoff here is worth (increased convenience vs. introducing possible bugs).

In terms of how we would do this, I'd prefer that we implement our own interface for this that does not risk exposing the BehaviorSubject API at all, and is essentially a extension of Observable that includes some additional methods

// this is invalid b/c Observable is a class, but you get the idea
interface LastValueObservable extends Observable<T> implements SubscriptionLike {
  // value may not be defined yet, we could either return undefined or
  // throw an error if this is called before a value has been emitted
  // (which would indicate this is being used with an API that does not have a 
  // replay effect)
  getLastValue(): T | undefined;
}

One thing to consider as well is that we generally expose Observables via a function that returns a new Observable. With this pattern, we would need to implement SubscriptionLike and require consumers to unsubscribe the observable they received. I'm not sure this is a good idea and could lead to memory leaks. This is true with the proposal above or with BehaviorSubject.

What we need to figure out though is whether or not the tradeoffs above are worth the risks. @streamich could you point to specific examples in our codebase where introducing this would significantly improve the implementation?

[pgayvallet]

I'll assume we don't need this anymore and close the issue. Please feel free to reopen otherwise.