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

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

Closed
wants to merge 8 commits into from

Conversation

petrpavlik
Copy link
Contributor

@petrpavlik petrpavlik commented Oct 6, 2019

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
Contributor Author

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

@petrpavlik
Copy link
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
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
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
Copy link
Member

RuiAAPeres commented Oct 25, 2019

@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
Contributor Author

@RuiAAPeres here's the separate travis PR #764

@petrpavlik
Copy link
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
Copy link
Member

andersio commented Nov 10, 2019

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

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
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
Copy link
Member

andersio commented May 22, 2020

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
Copy link
Contributor

mluisbrown commented Jun 5, 2020

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

@petrpavlik
Copy link
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
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
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
Copy link
Contributor Author

petrpavlik commented Jun 12, 2020

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.

None yet

5 participants