Expose observe overloads for separate tracking and application of changes

[maximkrouk]

Follow-up PR for #281

The issue:

In code like this:

parentComponent.bind(model)

where

ParentComponent {
  func bind(_ model: ParentModel) {
    observe {
      self.childComponent.bind(model.child)
    }
  }
}

ChildComponent {
  func bind(_ model: ChildModel) {
    observe {
      self.value = model.value
    }
  }
}

call stack will look kinda like this

ParentComponent.bind {
  observe { // #1
    ChildComponent.bind {
      observe { // #2
        // child props
      }
    }
  }
}

And since child props access is nested in observe { // #1, parent will apply self.childComponent.bind(model.child) on any child props change even tho only child should be updated in that case via observe { // #2

Proposed solution

Provide an observe overload that will track and apply changes separately so the pseudocode from above will look like this:

ParentComponent {
  func bind(_ model: ParentModel) {
    observe { _ = model.child } onChange: {
      self.childComponent.bind(model.child)
    }
  }
}

ChildComponent {
  func bind(_ model: ChildModel) {
    observe { // this call can actually stay the same simple observe
      self.value = model.value
    }
  }
}

call stack will look kinda like this

ParentComponent.bind {
  observe() // onChange will be triggered separately from tracking
}

ChildComponent.bind {
  observe { // #2
    // child props
  }
}

Final note

Basically the library adds a cool handling of UITransactions and resubscription to withPerceptionTracking but cuts down the ability to separate tracking and updates which is present in withPerceptionTracking. The solution cannot be replaced with a simple use of withPerceptionTracking since UITransaction-related stuff is library implementation detail, this PR keeps existing functionality, but brings back a lower-level withPerceptionTracking-like API keeping the cool stuff related to UITransaction. The API is not as ergonomic as a basic observe but it handles an important edgecase and it's sufficient for users of the library to build their own APIs based on the new method.

[maximkrouk]

@stephencelis @mbrandonw Looking forward for your review 🫠

[mbrandonw]

Hi @maximkrouk, I'm sorry but I still don't really understand what you're are trying to achieve here, and why it is any better than just using observe directly. Can you please provide a full, compiling example of something that makes use of these new tools?

Also, have you tried implementing these tools outside of the library? If not, can you try? And if you run into problems can you let us know what those are?

[maximkrouk]

Here is a simplified example in an executable swift package maximkrouk/swift-navigation-test-repo:simplified

The issue is that nested observe applictions are triggering parent ones. This behavior is expected because the withPerceptionTracking apply and onChange arguments are combined into a single apply argument in the observe function. However, we must use observe to have UITransaction features enabled, but current observe implementation robs us of the ability to utilize the derived apply and onChange provided separately by withPerceptionTracking.

Redundant updates are present for any amount of nested changes so it'll be a pretty bit problem in a larger application structure like this (pseudocode)

If we set

appView.setModel(appModel)

where

func setModel(_ model:) {
  observe {
    childView.setModel(model.child) // will call observe for child props and probably `setModel` for child.child etc.
  }
}

then

AppView { // ← this
  MainView { // ← this
    Header { // ← this
      Labels { // ← and this update will be triggered
        UsernameLabel() // ← for a simple text change here
      }
    }
  }
}

Implementing smth similar outside the lib requires jumping through hoops, I did smth similar utilizing @testable import as a proof of concept before preparing the PR but this solution it won't work in prod anyway (and it's still pretty bad, you can take a look at the repo, I extracted it to a separate file).

And last but not least I genuinely believe that adding withPerceptionTracking-like API it's just a common sense, not some super niche feature. This helper should be implemented in the library, yes it's a bit less ergonomic, but it does the job and allows to avoid redundant updates and also allows to implement convenience stuff (I will also wrap it and on app-level won't use it directly, however I don't see a good way to handle nested updates without this being merged)

Some examples of what people might do for convenience

// autoclosure-based observation
func observe<Value>(
  _ value: @Sendable @escaping @autoclosure () -> Value,
  onChange: @Sendable @escaping (Value) -> Void
) -> ObserveToken {
  SwiftNavigation.observe { _ = value() } onChange: {
    onChange(value())
  }
}

observe(myModel.text) { label.text = $0 }

// KeyPath-based observation with weak capture
func observeWeak<Object: AnyObject & Perceptible & Sendable, Value>(
  _ object: Object,
  _ keyPath: KeyPath<Object, Value> & Sendable,
  onChange: @Sendable @escaping (Value) -> Void
) -> ObserveToken {
  SwiftNavigation.observe { [weak object] in
    _ = object?[keyPath: keyPath]
  } onChange: { [weak object] in
    object.map { onChange($0[keyPath: keyPath]) }
  }
}

observeWeak(myModel, \.text) { label.text = $0 }

Oh and

why it is any better than just using observe directly

it's not just better, it allows to correctly process nested observables (this is possible with pure withPerceptionTracking btw, but it would kinda break UITransaction-related stuff since it's hidden in observe), which current observe doesn't allow 🌚

[maximkrouk]

I was trying to cover the issue extensively, but tldr is:

swift-navigation extends swift-perception with UITransaction but it removes an API for separate tracking and application of changes. It shouldn't be like this at the first place (extending and cutting out stuff on the same level) and this PR only brings back swift-perception-like API extended with UITransaction

Also the example contains external implementation of changes, I see it as hacky, unreliable and not usable in prod.
So it's impossible to implement proposed changes outside of swift-navigation

[stephencelis]

@maximkrouk Revisiting this I think we're down to merge this! I merged main into the branch, though, and looks like there are issues due to a refactor of observe that occurred recently for Swift 6.2 support. Think you can take a look soon?

[pyrtsa]

Why doesn't this function body ignore the tracking parameter? Could we have a unit test to check that this actually works?

[maximkrouk]

Indeed it has to be used and I fixed it in my local version a while ago, but didn't push it, I'll push an update tomorrow

[maximkrouk]

@maximkrouk Revisiting this I think we're down to merge this! I merged main into the branch, though, and looks like there are issues due to a refactor of observe that occurred recently for Swift 6.2 support. Think you can take a look soon?

Wow, it's been a while, but I'm glad anyway 😅

I'll take a look and push an update tomorrow

[maximkrouk]

I left a comment related to this branch here #306 (comment)

[stephencelis]

@maximkrouk Replied, but the short of it is you can ignore those warnings!

[maximkrouk]

I fixed comments and updated tests but now there are 2 other issues:

• Task.yeild is not enough to await for updates to take place, had to use Task.sleep in my tests for now
• Some other tests fail
  • IsolationTests: Assertion fails for both main and global actors
  • LifetimeTests: Relies on Task.yeild

I ran out of time today, so I'll take a look on it tomorrow, but from high level overview of my changes it doesn't look like it should break anything (especially for the old signature with one closure) 🤔

[maximkrouk]

The order of logs is not consistent here, so I decided to assert by the count which seems stable across runs

[maximkrouk]

UPD:

• I found that I missed some @isolated(any) attributes
• I also tried adding it kinda everywhere and marking task closures as async (didn't help, even tho isolation should've been preserved)
• ⚠️ Figured out that isolation tests are also failing on the main branch 🫠

Actually ☝️🤓 I thought that ActorProxy was a pretty neat solution and afaik it didn't have warnings (so no unexpected failures should occur on some future swift updates), maybe we should just revert it? 🤔

[maximkrouk]

The task here was breaking the isolation, maybe marking everything as @_inheritActorContext + @isolated(any) wasn't required, I'll check if it works without it later 🫠

[maximkrouk]

Btw Task.yield() is still not sufficient for awaiting for changes to be applied 😔

[maximkrouk]

Btw Task.yield() is still not sufficient for awaiting for changes to be applied 😔
I also noticed that old observe(_:) should use a separate version of onChange/withRecursivePerceptionTracking, current one is calling this shared apply closure too often I'll investigate it a bit later 🫠

[maximkrouk]

Btw Task.yield() is still not sufficient for awaiting for changes to be applied 😔

[maximkrouk]

Btw Task.yield() is still not sufficient for awaiting for changes to be applied 😔
I also noticed that old observe(_:) should use a separate version of onChange/withRecursivePerceptionTracking, current one is calling this shared apply closure too often I'll investigate it a bit later 🫠

[maximkrouk]

Workarounds to make @escaping @isolated(any) @Sendable () -> Void functions callable from sync contexts

TODO: remove @escaping

[stephencelis]

Actually ☝️🤓 I thought that ActorProxy was a pretty neat solution and afaik it didn't have warnings (so no unexpected failures should occur on some future swift updates), maybe we should just revert it? 🤔

It does look like we need to bring it back (#316) but we still want @isolated(any), which is required for some closures to successfully compile in Swift 6.2.