Add compatibilty with Swift's @Observable macro#526
Conversation
Views now update automatically when an `@Observable` model class used inside `body` changes. The same is true for `@Perceptible`. Adds a `@Bindable` property wrapper so that properties of observable models can be easily used as bindings.
|
Does this support |
|
@JoshBashed This PR tries to add compatibility with |
stackotter
left a comment
There was a problem hiding this comment.
Thank you for this PR 🙏 I particularly like the ViewModelObserver protocol that you've used to make observation more succinct.
I've left a bunch of comments requesting a variety of changes, the biggest being potentially splitting perceptible support into a separate PR so that I can merge Observable support faster.
There was one other thing that I didn't comment on with a review comment, because I didn't know where to attach it. I would like for us to be vend an @Observable implementation (borrowed from Swift itself) to support @Observable as our official observation solution across all OS versions supported by SwiftCrossUI. My guess is that that would involve a bunch of copy+pasting (with care taken to give correct attribution and separate it from our own code a bit), and fixing up the implementations to not require newer language features. Then if you do that we should @_reexport import Observation on platforms that do have Observation so that people don't have to conditionally import Observation themselves based on target platform/platform version. Let me know if any of that doesn't make sense or isn't feasible!
Thanks for the detailed documentation by the way!
| } | ||
| ModifyingView(model: model) | ||
| .padding() | ||
| if !model.automaticModeIsOn { |
There was a problem hiding this comment.
Can you please make it possible to disable automatic mode again without restarting the app? Will need to store a reference to the automatic task, and then can probably cancel it from an 'onChange(of: automaticModeIsOn)' handler
| /// `@Perceptible` from the | ||
| /// [Perception](https://github.com/pointfreeco/swift-perception) package. |
There was a problem hiding this comment.
If perception gets split into a separate PR, remember to update these doc comments
| } | ||
|
|
||
| extension Bindable : Identifiable where Value : Identifiable { | ||
|
|
There was a problem hiding this comment.
Remove this blank line
| .package( | ||
| url: "https://github.com/pointfreeco/swift-perception.git", | ||
| from: "2.0.10" | ||
| ), |
There was a problem hiding this comment.
I believe that swift-perception support should be behind a trait, as it won't be our recommended solution and is more of a convenience integration for people who want to use swift-perception anyway. Our Swift tools version is currently too low for traits, but it's getting to a point where we should probably consider bumping our tools version (there have been quite a few good reasons lately).
Would you feel comfortable splitting the swift-perception support into a separate PR so we can merge Observable support without having to worry about the Swift tools version bump?
| /// let body = self.observe(in: backend) { view.body } | ||
| /// // Use `body` | ||
| /// | ||
| /// Then, `viewModelDidChange()` will automatically be called the next time a view model conforming |
There was a problem hiding this comment.
/// Then, ``viewModelDidChange(backend:)`` ...
(that should link the docs to the method correctly)
| /// The dynamic property updater for this view. | ||
| private var dynamicPropertyUpdater: DynamicPropertyUpdater<NodeView> | ||
|
|
||
| /// Used by the `ViewModelObserver` protocol to prevent duplicate view updates. |
There was a problem hiding this comment.
Use double backticks to create a DocC symbol link
| implementation = StateImpl(initialStorage: Storage(initialValue)) | ||
| } | ||
|
|
||
| #if os(macOS) || os(iOS) || os(tvOS) || os(watchOS) || os(visionOS) |
There was a problem hiding this comment.
Just as a formatting thing we generally indent compile time conditionals to match surrounding code (with the body indented an additional level)
| implementation = StateImpl(initialStorage: Storage(initialValue)) | ||
| } | ||
|
|
||
| #if os(macOS) || os(iOS) || os(tvOS) || os(watchOS) || os(visionOS) |
There was a problem hiding this comment.
Given that this is an 'Apple platforms' issue, you could consider using #if canImport(Darwin) to be resilient against future Apple platforms being introduced (such as would've happened to this same code a couple of years ago when visionOS was introduced)
| import Foundation | ||
| import PerceptionCore | ||
|
|
||
| /// This protocol can be adoopted by classes responsible for handling part of the view hierarchy. It makes |
| } | ||
| } | ||
|
|
||
| extension Bindable where Value : AnyObject { |
There was a problem hiding this comment.
We only put a space after colons (and not before them). I think you've done this in a couple of other places so just double check the rest of the PR as well (can probably just command+f for :)
|
@stackotter Thank you for the kind words! And thanks for the detailed response. I'm happy to implement the changes. There's one thing I'm not 100% clear on and I want to clear that up before starting: You suggest removing swift-perception does all the heavy lifting behind the scenes: It allows
Option 1 would be by far the most complicated and maintenance-heavy option. And the only benefit over Option 4 that I can think of is that the project would have one fewer dependency. It would take a lot of work to implement and test and the added code would need maintenance. Option 2 would probably be relatively straightforward. We would lose the dependency but also the ability to observe on older OSs. Option 3 allows all users to use the observation functionality. If they use Option 4 has the same benefits as Option 3 but would allow users to use What do you think? |
|
Ah I didn't quite realise that swift-perception basically does exactly what I proposed with backporting the official Swift Observation implementation. I think option Are there any |
|
@stackotter Great, we're on the same page! I'll try to get to it next week. I believe swift-perception supports all APIs from |
Resolves #407.
This pull request does the following:
@Observableor@Perceptibleobjects used inside the view'sbodychange.Observabletypealias because it would clash with the new functionality.ObservationIgnoredmacro in OSs where the Observation framework is available. This fixes a problem where this macro would clash with theObservationIgnoredmacro from the Observation framework (see Add compatibilty with Swift's@Observablemacro #407). In my testing, this solution fully fixes the clash. In OSs where Observation is available, users may need toimport Observationto continue using@ObservationIgnoredtogether with@ObservableObject. The obsoletion error message also instructs users to do so.I have split the PR into two commits. The first one contains a straightforward implementation of the new functionality. The second one simplifies the implementation using a protocol. If you prefer a straightforward solution, the second commit could be dropped. It adds no new functionality.