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

Behaviour getValue functionality missing #758

Closed
alisd23 opened this issue Nov 20, 2015 · 9 comments
Closed

Behaviour getValue functionality missing #758

alisd23 opened this issue Nov 20, 2015 · 9 comments
Assignees

Comments

@alisd23
Copy link

alisd23 commented Nov 20, 2015

Is the value property of BehaviourSubject supposed to be public? Or is there supposed to be a public getValue function like there is in the previous version?
There is currently no way of reading the value without subscribing.

@Johnius
Copy link

Johnius commented Nov 20, 2015

I think it's breaking supposed way of reactive paradigm. Can you describe your specific case? Maybe there's better way to solve this problem.

@benlesh
Copy link
Member

benlesh commented Nov 20, 2015

Is the value property of BehaviourSubject supposed to be public?

Yes (and no). FWIW, you could examine that in previous versions as well. most of the time you shouldn't be using this though, but there are situations were it can come in handy.

So:

  • Yes, there it is.
  • But try not to use it.

Fair?

@chrisprice
Copy link
Contributor

My (potentially edge!) use case involves a source of deltas to a data structure. In order to apply the delta I need a reference to the existing data structure. E.g. -

private _x: BehaviorSubject<IX[]> = new BehaviorSubject([]);

get x() : Observable<IX[]> {
  return this._x;
}

private apply(delta: Delta<IX>): void {
  delta(this._x.value); // invalid access to private member
}

The commit which changes it from public to private doesn't provide any context.

My options are to -

  • maintain a separate reference to the current value (seems a bit unnecessary).
  • create a new subscription to retrieve the current value (seems a lot unnecessary).
  • cast around the type checks ((this._x as any).value as IX[]) (seems a bit ugly).

Any pointers to alternative approaches would be much appreciated. Also, FWIW I would rather see it be implemented as (happy to make a PR) -

import {Subject} from '../Subject';
import {Subscriber} from '../Subscriber';
import {Subscription} from '../Subscription';

export class BehaviorSubject<T> extends Subject<T> {
  constructor(private _value: T) {
    super();
  }

  _subscribe(subscriber: Subscriber<any>): Subscription<T> {
    const subscription = super._subscribe(subscriber);
    if (!subscription) {
      return;
    } else if (!subscription.isUnsubscribed) {
      subscriber.next(this._value);
    }
    return subscription;
  }

  _next(value: T): void {
    super._next(this._value = value);
  }

  get value(): T {
    return this._value;
  }
}

@benlesh
Copy link
Member

benlesh commented Nov 23, 2015

BehaviorSubject's value property should not have been made private. That's a good catch. getValue seems redundant of the value property is public, what do you think?

@chrisprice
Copy link
Contributor

I don't like a completely public value because it can then be set without
triggering the subscriber's next.

I was thinking that hiding it behind either a getValue or get valueprotects against this. However, in TypeScript you can get a similar
effect by returning a stricter interface and in JavaScript the _value
member would be directly accessible anyway.

I suppose the big advantage in both cases is to be more expressive about
how it should/should not be used. A future minifier might eventually
obfuscate the _value member by virtue of it being private.

Hm, I think I've managed to talk myself around 360 back to the
implementation I suggested above :)

On Mon, 23 Nov 2015 23:11 Ben Lesh notifications@github.com wrote:

BehaviorSubject's value property should not have been made private.
That's a good catch. getValue seems redundant of the value property is
public, what do you think?


Reply to this email directly or view it on GitHub
#758 (comment).

@benlesh
Copy link
Member

benlesh commented Nov 24, 2015

All I know is it's available via value in RxJS 4. That's generally where I've gotten it from. I'm not opposed to a getValue(), per say. But I think keeping value public to maintain that bit of compatability is probably a good idea.

@mattpodwysocki
Copy link
Collaborator

@Blesh calling .value is an anti-pattern for many reasons. If you look at getValue() as it stands today it does a number of things.

  1. Checks if the current BehaviorSubject has been disposed and throws if it is.
  2. If there was an error in the stream, then you cannot just get the value, instead the error is thrown.
  3. If all else is successful, then you may get the value.

Simply exposing .value breaks that encapsulation because you need those safeguards.

@benlesh
Copy link
Member

benlesh commented Nov 24, 2015

Okay... getValue() then.

@benlesh benlesh added the help wanted Issues we wouldn't mind assistance with. label Nov 24, 2015
@benlesh benlesh self-assigned this Dec 1, 2015
@benlesh benlesh removed the help wanted Issues we wouldn't mind assistance with. label Dec 1, 2015
@benlesh benlesh closed this as completed in 33b387b Dec 8, 2015
benlesh added a commit that referenced this issue Dec 8, 2015
this class was added to support throwing errors when members of Subjects were accessed in invalid ways after the Subject has been unsubscribed

related #859
related #758
benlesh added a commit that referenced this issue Dec 8, 2015
@lock
Copy link

lock bot commented Jun 7, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants