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

feat(shareBehavior): add shareBehavior and its tests #588

Closed
wants to merge 1 commit into from

Conversation

staltz
Copy link
Member

@staltz staltz commented Oct 23, 2015

No description provided.

@benlesh
Copy link
Member

benlesh commented Oct 26, 2015

This is bike-shedding, but wasn't this called "shareValue" before? Is that a better term? What would make the most sense for people new to the library? (FWIW: I found the name of "BehaviorSubject" to be essentially nonsense when I first started using Rx)

@benlesh
Copy link
Member

benlesh commented Oct 26, 2015

Also, wasn't there a problem with shareBehavior, @staltz? Or was that only for shareReplay? (The problem being that it creates a new Subject in it's multicast)

@staltz
Copy link
Member Author

staltz commented Oct 27, 2015

This is bike-shedding, but wasn't this called "shareValue" before? Is that a better term?

It was, but we discussed here and reached a consensus to align the sharing names to their respective subjects, for symmetry:

  • share uses Subject
  • shareReplay uses ReplaySubject
  • shareBehavior uses BehaviorSubject

To rename it to shareValue would mean also renaming BehaviorSubject to ValueSubject. I'm not contesting that actually. My brain got used to BehaviorSubject as a name, and perhaps to people migrating to v5 it would be easier to keep the same names, but I actually agree that ValueSubject would be more intuitive.

Also, wasn't there a problem with shareBehavior, @staltz? Or was that only for shareReplay?

Both shareBehavior and shareReplay "have problems" (this issue), and these tests are not covering those problems yet. So this PR here is agnostic to whatever happens to our decision regarding issue #453. When that's decided, we need to add more tests for these sharing operators.

@benlesh
Copy link
Member

benlesh commented Oct 27, 2015

Okay, thanks for the clarification. Looking over this in more detail it looks fine. Sorry, I was going through 30+ PRs or something yesterday. LOL.

@benlesh
Copy link
Member

benlesh commented Oct 27, 2015

merged with 97ff1ec, thanks @staltz

@benlesh benlesh closed this Oct 27, 2015
@staltz staltz deleted the shareBehavior branch October 27, 2015 17:50
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
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 this pull request may close these issues.

2 participants