Add AnyVariable (get-only Variable) by inamiy · Pull Request #1118 · ReactiveX/RxSwift

inamiy · 2017-02-28T00:30:26Z

This PR adds AnyVariable as a get-only Variable, which is highly useful when encapsulating Variable inside some view model, disallowing its binding from outside.

This type is almost the same as ReactiveSwift's Property.

RxPullRequestBot · 2017-02-28T00:32:29Z

	`2 Warnings </th> </tr>`
⚠️	Please target PRs to `develop` branch
⚠️	No CHANGELOG changes made

Generated by 🚫 danger

kzaher

Hi @inamiy ,

Thnx for making this PR. I've seen quite a few of your projects, and it looks like you have similar interests :)

Unfortunately I don't think we should import this PR. I can definitely see how something like this could be useful

... which is highly useful when encapsulating Variable inside some view model, disallowing its binding from outside.

... but I don't think the core project should be optimized for MVVM pattern.

I think I'm probably to blame a bit for the Variablegate scandal. The reason why I've added Variable to RxSwift because finding BehaviorSubject is just excruciatingly hard and it was affecting adoption of this library.

After that people started to use it for MVVM, and I guess that made sense since we've added some additional guarantees, but I would like to stop there.

It's a tiny class that gives us pretty big bang for a buck by significantly helping new users but not deviating too much from cross platform implementation. And when one looks at its implementation, it's just a simple wrapper around BehaviorSubject, so there is another learning opportunity there.

I think it would maybe make more sense to add a new RxMVVM project to RxSwiftCommunity since there was already a request like this in the past.

My suggestion would be to create a unit named Value that has all of the Driver unit guarantees + additional guarantee that it will always emit a value synchronously upon subscription.

So one can convert Driver to Value by just applying startWith operator or directly from Variable to Value without any additional information.

kzaher · 2017-02-28T22:54:18Z

RxSwift/Subjects/Variable.swift

 /// Unlike `BehaviorSubject` it can't terminate with error, and when variable is deallocated
 /// it will complete it's observable sequence (`asObservable`).
-public final class Variable<Element> {
+public final class Variable<Element>: ObservableConvertibleType {


This is actually on purpose.

I didn't want somebody to write extension for ObservableConvertibleType and accidentally retain Variable.

Not conforming it to ObservableConvertibleType isn't perfect solution for that since anybody can make it conform, but it should reduce the chance a bit.

¯\_(ツ)_/¯

I got your point, but I just wonder if ObservableConvertibleType will ever be a useful protocol for 3rd party users 😅

Hi there, I'm interested that why Variable does not conform to ObservableConvertibleType and ObserverType.

In my opinion, I think that the type A that implemented ObservableConvertibleType make B as Observable and some other object should retain B object, not A object directly.
Then Variable case, the Observable is a internal BehaviorSubject so, Variable never be retained other object.

But now, maybe some of Observable family has ObservableConvertibleType directly. Can we change this?

Hi @tarunon ,

the mere fact that we are relying on the fact will somebody retain variable or not means that this is fragile API (the part about completing variable observable sequence on deinit). That's why I don't like it.

It is pretty normal to create a reference to extension source, being it ObservableType or ObservableConvertibleType. This is being done all the time, and it shouldn't be a problem.

That's why Variable doesn't conform to ObservableConvertibleType, because that increases the chances of somebody retaining it.

Every solution has it's drawbacks.

@tarunon
I'm assuming that observable.flatMap { _ in variableFromOuterScope } will be a beginner mistake that should be avoided.

BTW for ObserverType, I guess Variable can't conform to this protocol because of NoError restriction which protocol itself can't guarantee.

Thanks for the discussion.
Finally, I can understand that why Variable doesn't conform to ObservableConvertibleType.

@inamiy
About ObserverType, I think that is not a real problem. For example, UIBindingObserver has a same implement that func bindTo(_ variable: Variable<E>) -> Disposable's one.

If my understanding is correct, adding ObserverConvertibleType may help us.
I think it is effective not only Variable.

(Just for memo) Variable should not conform to ObserverType because terminal events will be ignored, i.e. variable.on(.completed)) then variable.on(.next(value)) will send value. (And using fatalError inside is not an option...)

kzaher · 2017-03-01T08:23:34Z

I guess it could also make sense to add Value unit to RxCocoa.

inamiy · 2017-03-01T16:49:24Z

Hi @kzaher, thank you for mention :)

If I understand correctly:

Value (get-only Variable) is not related to Driver or threading, as Variable isn't.
Value doesn't require any Cocoa-related types, as Variable doesn't. So I think RxSwift (not RxCocoa) is a better place to live in.
The type name Value has a bit of difficulty when one wants to get current element, i.e. value.value

kzaher · 2017-03-02T12:46:00Z

Hi @inamiy ,

Value (get-only Variable) is not related to Driver or threading, as Variable isn't.

I think it should be tied because it's a safer alternative. The only environment where that unit makes sense is a stateful one (like client app).

Since Driver is just a specialization of SharedSequence, so should probably Value be a typealias of some more general concept that always has latest value available and ensures being executed on some serial scheduler.

Since SharedSequence already ensures scheduler (when converting using startWith), then the only real difference is one additional observeOn when converting from Variable to ensure all properties are satisfied.

Value doesn't require any Cocoa-related types, as Variable doesn't. So I think RxSwift (not RxCocoa) is a better place to live in.

I don't think that in general dependency chain should be the only reason to decide how public API should look like. If that was the case then we should also make Queue and PriorityQueue public.

Because:

to introduce this unit it should have a general enough use cases
it should have a private initializer
it should be able to create it from Variable(asValue) and Driver/SharedSequence(asValue), and SharedSequence is defined in RxCocoa
all code in RxSwift should be cross platform

I think it would be more suitable to add it to RxCocoa.

The type name Value has a bit of difficulty when one wants to get current element, i.e. value.value

I don't think that it should have an API that allows access to its latest element, just that it should guarantee that initial value is always delivered immediately upon subscription.

Having a value property would mean that the concept is imperative in nature vs declarative like the rest of library. That would be a road to complexity IMHO.

inamiy · 2017-03-02T15:49:45Z

@kzaher
I should have emphasized that Value is just a wrapper of Variable so all of the next scenarios should follow the same as Variable, i.e.

Value is also a cross-platform, and should be place in RxSwift (not RxCocoa)
Can add Value+Driver extension in RxCocoa if you want, but not mandatory.

BTW:

I don't think that it should have an API that allows access to its latest element, just that it should guarantee that initial value is always delivered immediately upon subscription.

Value as "Variable without var value { get set }" sounds an interesting idea!
The naming problem will be solved if this idea is applied.

kzaher · 2017-03-04T11:20:47Z

Hi @inamiy ,

I should have emphasized that Value is just a wrapper of Variable so all of the next scenarios should follow the same as Variable, i.e.

Yep, got it :) I'm not trying to say is it useful or not, I'm trying to say that there are a lot more criteria that need to be satisfied for us to pull something in RxSwift project.

Value is also a cross-platform, and should be place in RxSwift (not RxCocoa)

When I use term term cross platform, I'm not referring to iOS, macOS, tvOS, .... but to RxJS, RxJava, RxRust, RxGo ... Value would be optimized mostly for UI code. The place where it would fit best is it's own separate repository with all of the other extension units, but it's more convenient to have them in RxCocoa for now.

Everything that is entering RxSwift core library should strive to be aligned with the core observable sequence interface/concept (observable sequence compatible), strive to follow cross platform implementations, have no external dependencies, promote declarative style, be useful enough for us to maintain it here (maintaining Swift code is not cheap), align with future plans (potentially adding Value to RxCocoa) ...

It would be extremely easy and popular choice to add a lot of convenience extensions in there for us to maintain, but it's extremely hard to get them out or actually maintain them.

inamiy · 2017-03-06T13:50:02Z

@kzaher
OK, I understood what you meant by "cross-platform".
But how about Variable? It is also uncommon in ReactiveX, although in core RxSwift.framework.
To me, putting Value into RxCocoa is very inconsistent, and I'm not positive to fix in that way.

I would like to hear if anyone else is 👍 or 👎 to this PR.
I'm also okay with this PR closed and someone takes over to target RxCocoa instead of me.

kzaher · 2017-03-08T23:45:53Z

Ok, cool.

inamiy · 2017-03-11T00:36:42Z

FYI I open-sourced https://github.com/inamiy/RxProperty .

inamiy added 2 commits February 28, 2017 09:23

Add AnyVariable (get-only Variable)

c202e76

Follow guideline

8460a39

inamiy changed the base branch from master to develop February 28, 2017 01:03

kzaher reviewed Feb 28, 2017

View reviewed changes

kzaher mentioned this pull request Mar 4, 2017

Feature/zip with #1120

Closed

kzaher closed this Mar 8, 2017

kzaher mentioned this pull request May 5, 2017

Read-only version of Variable type #1236

Closed

3 tasks

4brunu mentioned this pull request Jan 30, 2019

Consider publish this library into CocoaPods/Carthage inamiy/RxProperty#7

Open

Conversation

inamiy commented Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RxPullRequestBot commented Feb 28, 2017

Uh oh!

kzaher left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kzaher Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

inamiy Mar 1, 2017

Choose a reason for hiding this comment

Uh oh!

tarunon Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

kzaher Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

inamiy Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

tarunon Mar 3, 2017

Choose a reason for hiding this comment

Uh oh!

inamiy Mar 6, 2017

Choose a reason for hiding this comment

Uh oh!

kzaher commented Mar 1, 2017

Uh oh!

inamiy commented Mar 1, 2017

Uh oh!

kzaher commented Mar 2, 2017

Uh oh!

inamiy commented Mar 2, 2017

Uh oh!

kzaher commented Mar 4, 2017

Uh oh!

inamiy commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kzaher commented Mar 8, 2017

Uh oh!

inamiy commented Mar 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

inamiy commented Feb 28, 2017 •

edited

Loading

kzaher left a comment •

edited

Loading

inamiy commented Mar 6, 2017 •

edited

Loading