Skip to content

Add @Observable property wrapper and bump the CI to Xcode 11 / Swift 5.1 - #762

Closed
petrpavlik wants to merge 8 commits into
ReactiveCocoa:masterfrom
petrpavlik:master
Closed

petrpavlik wants to merge 8 commits into
ReactiveCocoa:masterfrom
petrpavlik:master

Conversation

@petrpavlik

@petrpavlik petrpavlik commented Oct 6, 2019 •

Copy link
Copy Markdown
Contributor

Checklist

  • Updated CHANGELOG.md.

turn this

class ViewModel {
    private(set) lazy var profile = Property<Profile>(mutableProfile)
    private let mutableProfile: MutableProperty<Profile>

    init(profile: Profile) {
        mutableProfile = MutableProperty<Profile>(profile)
    }
}

into this

class ViewModel {
    @Observable private(set) var profile: Profile

    init(profile: Profile) {
        self.profile = profile
    }
}

usage

viewModelInstance.profile.$property.signal.observeValues { value in
    // ....
{

@petrpavlik

Copy link
Copy Markdown
Contributor Author

I'm working on fixing the CI after bumping to Xcode 11

@petrpavlik

Copy link
Copy Markdown
Contributor Author

ok, ready

@petrpavlik
petrpavlik marked this pull request as ready for review October 13, 2019 20:56
@petrpavlik petrpavlik mentioned this pull request Oct 13, 2019
@RuiAAPeres

Copy link
Copy Markdown
Member

This is nice, just a couple of things:

  1. Maybe split the PR so that CI fix and bump live in a separate PR. It seems that this has been done.
  2. I am not too sure about the naming Observable.
  3. You are using a MutableProperty internally. I wonder what's the benefit of having a Property at all in this case.

@petrpavlik

Copy link
Copy Markdown
Contributor Author

The diff of this PR shows that the CI config hasn't been upgraded yet, but I'm happy to slit in into a separate PR.

@RuiAAPeres

RuiAAPeres commented Oct 25, 2019 •

Copy link
Copy Markdown
Member

@petrpavlik very true and I am wrong. The fix for Xcode 11 and Swift 5.1 was only for the code, not the actual CI. This tipped me off in the wrong direction #739.

@petrpavlik

Copy link
Copy Markdown
Contributor Author

@RuiAAPeres here's the separate travis PR #764

@petrpavlik

Copy link
Copy Markdown
Contributor Author

Regarding the other comments:

  1. I'm easy when it comes to what this should be called. Any suggestions since you're obviously more familiar with naming conventions of ReactiveSwift?

  2. Consider following example:

private class TestCounter {
         @Observable private(set) var value: Int = 0

         func increment() {
             value += 1
         }
     }

MutablePeoperty is only for internal use, if I exposed it publicly, you could do something like counter.value.$property.value = 1, we don't want to allow that since the property is private(set). That's why there's additional Property(mutableProperty), that is immutable and publicly exposed, so you can subscribe to it.

@andersio

andersio commented Nov 10, 2019 •

Copy link
Copy Markdown
Member

I'd recommend relaxing PropertyProtocol to drop the reference type constraint. With that, you can conform the property wrapper to MutablePropertyProtocol directly.

@robertjpayne

Copy link
Copy Markdown

This implementation still needs work to implement copy-on-write to avoid access control leakage.

You can currently do this to get around the "private" mutability:

var observable = self.viewModel.$profile.projectedValue
observable.wrappedValue = newProfile

Which will update the existing wrapped property as well.

@mluisbrown

Copy link
Copy Markdown
Contributor

Whats the status of this PR? Something like the @Published property wrapper in Combine would be really useful. In the meantime I'm having to roll my own.

@andersio

andersio commented May 22, 2020 •

Copy link
Copy Markdown
Member

Copy-on-write is generally inapplicable to any push based streams — each stream somewhat represents a unique topic, and subscribers hook into a specific unique topic. Therefore, copying becomes an alien concept to them because of the uniqueness constraint.

For example, what should happen to the existing subscribers, when a new value is sent through different copies of the same origin? Depending on the answer, we have outcomes varying from "might as well keep it as reference type" to "unimplementable in today's Swift".

Alternatively, we can strive to replicate the Combine @Published and @ObservableObject behaviours — this model dodges the issue above by forcing @Published to be usable only in classes. The only caveat is that it relies on a compiler private feature, i.e., enclosing self access in property wrappers. So it bears the same question as #731 in whether or not ReactiveSwift should use these officially unsupported features.

In any case, it isn't fair that progress on this front has been stuck, and that our user base cannot enjoy the improved brevity of property wrapper. So I've proposed a substitute (#781) that need not wait on the said design & maintenance decisions.

@mluisbrown

mluisbrown commented Jun 5, 2020 •

Copy link
Copy Markdown
Contributor

I guess this issue was resolved by #781? Should the PR be closed?

@petrpavlik

Copy link
Copy Markdown
Contributor Author

@mluisbrown I really like #781, but there's a catch.

My view models generally look like this

class ViewModel {
    private(set) lazy var profile = Property<Profile>(mutableProfile)
    private let mutableProfile: MutableProperty<Profile>

    init(profile: Profile) {
        mutableProfile = MutableProperty<Profile>(profile)
    }
}

this provide compiler guarantees that no code outside of the view model can assign a value to profile in this case. I don't think this can be achieved when using MutableProperty as a property wrapper.

class ViewModel {
    @MutableProperty private(set) var count: Int = 0
}

 let vm = ViewModel()

 // vm.count = 0 // won't compile
 vm.$count.value = 1 // will compile

As much as I'd love to migrate my code to this since it'd make things much simpler, I'm not willing to do it at the expense of losing this compile-time guarantee. My PR had this handled by only exposing Property, not MutableProperty.

@mluisbrown

Copy link
Copy Markdown
Contributor

@petrpavlik you're implementation also has the same problem with private mutability, albeit by a more convoluted route as pointed out in @robertjpayne 's comment:

You can do:

var observable = self.viewModel.$profile.projectedValue
observable.wrappedValue = newProfile

@petrpavlik

Copy link
Copy Markdown
Contributor Author

Ah, sorry, I missed that comment. Yeah, I'm happy to close this PR and bring up that access control leakage in a separate issue.

@petrpavlik

petrpavlik commented Jun 12, 2020 •

Copy link
Copy Markdown
Contributor Author

Closing this in favor of #781. The leakage issue is being tracked here #795

@petrpavlik petrpavlik closed this Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants