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

Moving *Relay into their own Framework. #1501

Open
sandeeplearner opened this Issue Nov 23, 2017 · 14 comments

Comments

Projects
None yet
7 participants
@sandeeplearner

sandeeplearner commented Nov 23, 2017

No deprecation warning in Xcode on using Variable:

As per release notes provided here, Variable is deprecated in the favor of BehaviorRelay. But I dont see any deprecation warning on using Variable in Xcode

Expected outcome:
Xcode should alert the deprecation of Variable.

What actually happens:
I am using RxSwift4. My pod specs looks like

pod 'RxSwift',    '~> 4.0'
pod 'RxCocoa',    '~> 4.0'

My project is making use of Swift4. When I declare a variable

var emyFilter = Variable<[MyFilterModel]>([MyFilterModel(data: "{:}")])

No warning is shown, realized the deprecation of Variable only when somebody specified it in their answer on SO

Just realized that I cant even access BehaviorRelay with RxSwift (4.0.0) in pod repo. Am I missing anything here? Is BehaviorRelay available in RxSwift4.0 and what should be our course of action? Should we still stick with Variable or BehaviorRelay? There are loads of content on web regarding Variable but dont see much on BehaviorRelay yet, so little confused of its usage as well. As I cant access it in my Xcode, cant even access the source code

RxSwift/RxCocoa/RxBlocking/RxTest version/commit

Using RxCocoa (4.0.0)
Using RxSwift (4.0.0)

Platform/Environment

  • iOS
  • macOS
  • tvOS
  • watchOS
  • playgrounds

How easy is to reproduce? (chances of successful reproduce after running the self contained code)

  • easy, 100% repro
  • sometimes, 10%-100%
  • hard, 2% - 10%
  • extremely hard, %0 - 2%

Xcode version:

  Version 9.1 (9B55)

Installation method:

  • CocoaPods
  • Carthage
  • Git submodules

I have multiple versions of Xcode installed:
(so we can know if this is a potential cause of your issue)

  • yes (which ones)
  • no

Level of RxSwift knowledge:
(this is so we can understand your level of knowledge
and formulate the response in an appropriate manner)

  • just starting
  • I have a small code base
  • I have a significant code base
@kzaher

This comment has been minimized.

Show comment
Hide comment
@kzaher

kzaher Nov 26, 2017

Member

Hi @sandeeplearner ,

As per release notes provided here, Variable is deprecated in the favor of BehaviorRelay. But I dont see any deprecation warning on using Variable in Xcode

Yes, so far we haven't put deprecation warnings on it since that API was pretty heavily used and I wanted to ease the 4.0 migration path.

We are currently preparing it for deprecation, but if somebody sees it there they will be incline not to use it.

Another reason why we haven't add deprecation warnings is because a clear deprecation path isn't decided yet, the only thing that is decided is that that concept will be deprecated in RxSwift target because:

  • it's not a standard cross platform concept so it's out of place in RxSwift target.
  • it doesn't have an extensible counterpart for event management (PublishRelay). It models state only.
  • it is naming is not consistent with *Relay
  • it has an inconsistent memory management model compared to other parts of RxSwift (complete on dealloc)

On the other side:

  • it helped new users with getting onboard more quickly (Behavior naming is novice user unfriendly)
  • unfortunately it was pretty heavily used concept

The current plan is this:

  • firstly move it in Deprecated.swift
  • figure out how to best sanitize the problems without destroying all the good sides
    • so far the best idea seems to be: moving it to RxCocoa, typealias Variable = BehaviorRelay and removing deallocation completion.
  • add deprecation warnings
  • remove Variable from RxSwift in future versions.

So far we are at: figure out how to best sanitize the....

I'll probably add this issue to Variable comments so this plan is publicly visible and users are better informed.

Member

kzaher commented Nov 26, 2017

Hi @sandeeplearner ,

As per release notes provided here, Variable is deprecated in the favor of BehaviorRelay. But I dont see any deprecation warning on using Variable in Xcode

Yes, so far we haven't put deprecation warnings on it since that API was pretty heavily used and I wanted to ease the 4.0 migration path.

We are currently preparing it for deprecation, but if somebody sees it there they will be incline not to use it.

Another reason why we haven't add deprecation warnings is because a clear deprecation path isn't decided yet, the only thing that is decided is that that concept will be deprecated in RxSwift target because:

  • it's not a standard cross platform concept so it's out of place in RxSwift target.
  • it doesn't have an extensible counterpart for event management (PublishRelay). It models state only.
  • it is naming is not consistent with *Relay
  • it has an inconsistent memory management model compared to other parts of RxSwift (complete on dealloc)

On the other side:

  • it helped new users with getting onboard more quickly (Behavior naming is novice user unfriendly)
  • unfortunately it was pretty heavily used concept

The current plan is this:

  • firstly move it in Deprecated.swift
  • figure out how to best sanitize the problems without destroying all the good sides
    • so far the best idea seems to be: moving it to RxCocoa, typealias Variable = BehaviorRelay and removing deallocation completion.
  • add deprecation warnings
  • remove Variable from RxSwift in future versions.

So far we are at: figure out how to best sanitize the....

I'll probably add this issue to Variable comments so this plan is publicly visible and users are better informed.

kzaher added a commit that referenced this issue Nov 26, 2017

@sandeeplearner

This comment has been minimized.

Show comment
Hide comment
@sandeeplearner

sandeeplearner Nov 27, 2017

@kzaher : Thanks a lot 👍

sandeeplearner commented Nov 27, 2017

@kzaher : Thanks a lot 👍

@DevAndArtist

This comment has been minimized.

Show comment
Hide comment
@DevAndArtist

DevAndArtist Dec 5, 2017

@kzaher one pain point migrating Variable to BehaviorRelay was that value is read-only, this disallows in-place mutation of the value - especially if the value is some kind of a collection. This requires a workaround like:

var mutableCopy = relay.value
mutableCopy.mutateSomehow()
relay.accept(mutableCopy)

DevAndArtist commented Dec 5, 2017

@kzaher one pain point migrating Variable to BehaviorRelay was that value is read-only, this disallows in-place mutation of the value - especially if the value is some kind of a collection. This requires a workaround like:

var mutableCopy = relay.value
mutableCopy.mutateSomehow()
relay.accept(mutableCopy)
@kzaher

This comment has been minimized.

Show comment
Hide comment
@kzaher

kzaher Dec 5, 2017

Member

@DevAndArtist

var mutableCopy = relay.value
mutableCopy.mutateSomehow()
relay.accept(mutableCopy)

this is a dangerous patten because after you read relay.value a new value could be set before you call relay.accept(mutableCopy) and thus you could accidentally overwrite something.

I am assuming people were doing this a lot, and because of Swift accessor semantics these issues where hidden, but I would say this is an anti-pattern for the mentioned reasons.

That's why we haven't made Variable deprecated yet. Even though I know it's theoretically correct thing to do, people have a lot of faulty existing code that works for them because of some implicit assumptions.

For example, if you assume all code is executed on main thread, then this can't happen

this is a dangerous patten because after you read relay.value a new value could be set before you call relay.accept(mutableCopy) and thus you could accidentally overwrite something.

But there is nothing in the API that prevents it.

We'll probably at least move Variable to RxCocoa in the next major version as a good first step.

Member

kzaher commented Dec 5, 2017

@DevAndArtist

var mutableCopy = relay.value
mutableCopy.mutateSomehow()
relay.accept(mutableCopy)

this is a dangerous patten because after you read relay.value a new value could be set before you call relay.accept(mutableCopy) and thus you could accidentally overwrite something.

I am assuming people were doing this a lot, and because of Swift accessor semantics these issues where hidden, but I would say this is an anti-pattern for the mentioned reasons.

That's why we haven't made Variable deprecated yet. Even though I know it's theoretically correct thing to do, people have a lot of faulty existing code that works for them because of some implicit assumptions.

For example, if you assume all code is executed on main thread, then this can't happen

this is a dangerous patten because after you read relay.value a new value could be set before you call relay.accept(mutableCopy) and thus you could accidentally overwrite something.

But there is nothing in the API that prevents it.

We'll probably at least move Variable to RxCocoa in the next major version as a good first step.

@DevAndArtist

This comment has been minimized.

Show comment
Hide comment
@DevAndArtist

DevAndArtist Dec 5, 2017

@kzaher even if it's a dangerous pattern in a multi-thread context, how would I append a new item to the value if for instance it was BehaviorRelay<[Int]>? What would be correct way to do so? I mean isn't that why we need a thread safe value { get set } for in-place mutation?

DevAndArtist commented Dec 5, 2017

@kzaher even if it's a dangerous pattern in a multi-thread context, how would I append a new item to the value if for instance it was BehaviorRelay<[Int]>? What would be correct way to do so? I mean isn't that why we need a thread safe value { get set } for in-place mutation?

@sandeeplearner

This comment has been minimized.

Show comment
Hide comment
@sandeeplearner

sandeeplearner Dec 6, 2017

@kzaher and @DevAndArtist : I had exact same issue which I had posted in SO. https://stackoverflow.com/questions/47452582/how-to-use-behaviorrelay-as-an-alternate-to-variable-in-rxswift

One solution proposed by community was to use BehaviorSubject but that defeats the purpose. BehaviorRelay is an alternate to Variable andVariable's value property was mutable now that BehaviorRelay has read only value. I ended up using accept as below

    let relay = BehaviorRelay(value: [10])
   //When I need to update my relay with new value
    let array = relay.value
    let newArray = Array(Set(array + [20,30]))
    relay.accept(newArray)

Though it works, I dont think this is the best way to use it. I am sure many are facing this confusion. Please add a sample usage of BehaviorRelay to clarify all our doubts.

sandeeplearner commented Dec 6, 2017

@kzaher and @DevAndArtist : I had exact same issue which I had posted in SO. https://stackoverflow.com/questions/47452582/how-to-use-behaviorrelay-as-an-alternate-to-variable-in-rxswift

One solution proposed by community was to use BehaviorSubject but that defeats the purpose. BehaviorRelay is an alternate to Variable andVariable's value property was mutable now that BehaviorRelay has read only value. I ended up using accept as below

    let relay = BehaviorRelay(value: [10])
   //When I need to update my relay with new value
    let array = relay.value
    let newArray = Array(Set(array + [20,30]))
    relay.accept(newArray)

Though it works, I dont think this is the best way to use it. I am sure many are facing this confusion. Please add a sample usage of BehaviorRelay to clarify all our doubts.

@Herbal7ea

This comment has been minimized.

Show comment
Hide comment
@Herbal7ea

Herbal7ea Dec 12, 2017

With this change, does BehaviorRelay have to be in RxCocoa? When using this in layers beyond the view (e.g., Presenter/MVVM/Model layers), the best practice is to not include UI imports, but RxCocoa has a lot of UI aspects. Variable was a simple to use piece, that only required RxSwift, but BehaviorRelay breaks that paradigm in it's current library.

Herbal7ea commented Dec 12, 2017

With this change, does BehaviorRelay have to be in RxCocoa? When using this in layers beyond the view (e.g., Presenter/MVVM/Model layers), the best practice is to not include UI imports, but RxCocoa has a lot of UI aspects. Variable was a simple to use piece, that only required RxSwift, but BehaviorRelay breaks that paradigm in it's current library.

@freak4pc

This comment has been minimized.

Show comment
Hide comment
@freak4pc

freak4pc Jan 9, 2018

Collaborator

@Herbal7ea The problem is Variable in itself isn't part of RxSwift or ReactiveX. It is a Cocoa-specific implementation that mainly works as an Imperative Bridge to people that find it hard to go "All declarative / all observable" immediately. It is not really a pure component of RxSwift to begin with 🤔

Collaborator

freak4pc commented Jan 9, 2018

@Herbal7ea The problem is Variable in itself isn't part of RxSwift or ReactiveX. It is a Cocoa-specific implementation that mainly works as an Imperative Bridge to people that find it hard to go "All declarative / all observable" immediately. It is not really a pure component of RxSwift to begin with 🤔

@kzaher

This comment has been minimized.

Show comment
Hide comment
@kzaher

kzaher Jan 13, 2018

Member

@Herbal7ea

If we extract *Relay and SharedSequence into their own frameworks in the next major version would that solve your concern?

The earliest time we can do this is the next major release because of backwards compatibility.

Member

kzaher commented Jan 13, 2018

@Herbal7ea

If we extract *Relay and SharedSequence into their own frameworks in the next major version would that solve your concern?

The earliest time we can do this is the next major release because of backwards compatibility.

@Herbal7ea

This comment has been minimized.

Show comment
Hide comment
@Herbal7ea

Herbal7ea Jan 17, 2018

@freak4pc That makes sense. I am mostly concerned about where things are located (RxCocoa), and not the why. @kzaher yeah, that would work. My hopes are mostly to separate the Variable alternative (BehaviorRelay) from any UI grouping. RxCocoa seems to be mostly geared toward working with UI pieces. Thank you both @freak4pc & @kzaher.

Herbal7ea commented Jan 17, 2018

@freak4pc That makes sense. I am mostly concerned about where things are located (RxCocoa), and not the why. @kzaher yeah, that would work. My hopes are mostly to separate the Variable alternative (BehaviorRelay) from any UI grouping. RxCocoa seems to be mostly geared toward working with UI pieces. Thank you both @freak4pc & @kzaher.

@nhatlee

This comment has been minimized.

Show comment
Hide comment
@nhatlee

nhatlee Mar 2, 2018

Hi all, please help me the right way to append value for BehaviorRelay . Is the solution of @sandeeplearner correct? I expected another way, because it seem verbose.

nhatlee commented Mar 2, 2018

Hi all, please help me the right way to append value for BehaviorRelay . Is the solution of @sandeeplearner correct? I expected another way, because it seem verbose.

@mortenbekditlevsen

This comment has been minimized.

Show comment
Hide comment
@mortenbekditlevsen

mortenbekditlevsen Apr 13, 2018

Hi @kzaher,
Regarding your comment about extracting *Relay and SharedSequence into their own framework from Jan. 13. :
Is this something that you consider doing?
I would welcome the change very much. :-)

mortenbekditlevsen commented Apr 13, 2018

Hi @kzaher,
Regarding your comment about extracting *Relay and SharedSequence into their own framework from Jan. 13. :
Is this something that you consider doing?
I would welcome the change very much. :-)

@kzaher

This comment has been minimized.

Show comment
Hide comment
@kzaher

kzaher Apr 13, 2018

Member

@mortenbekditlevsen It will be done in 5.0 which will target Swift 5.0, so it's tied with Swift 5.0 release date.

Member

kzaher commented Apr 13, 2018

@mortenbekditlevsen It will be done in 5.0 which will target Swift 5.0, so it's tied with Swift 5.0 release date.

@mortenbekditlevsen

This comment has been minimized.

Show comment
Hide comment
@mortenbekditlevsen

mortenbekditlevsen Apr 13, 2018

@kzaher That's perfect! :-) Thanks for the update!

mortenbekditlevsen commented Apr 13, 2018

@kzaher That's perfect! :-) Thanks for the update!

@kzaher kzaher changed the title from No deprecation warning on using Variable to Moving *Relay into their own Framework. Jun 7, 2018

@kzaher kzaher added the rxswift 5.0 label Jun 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment