Allow lifecycle callbacks to safety trigger re-renders #425
Conversation
|
|
||
| var temporaryWindow: UIWindow? = nil | ||
|
|
||
| func rootViewController() -> UIViewController { |
There was a problem hiding this comment.
The two cases in here ensure we can run the tests in both cocoapods world, which provides an app host, and SPM, which does not (there is no option in SPM to provide an app host).
There was a problem hiding this comment.
Hmm, are you sure this is necessary? The existing lifecycle tests work fine without an app host.
There was a problem hiding this comment.
I'm trying to make it work/exist as close to as possible to a "real" UI – hence pulling in the app host changes. While it passed before, there's enough special UIKit behavior that exists when being in a real app that I'd rather we just mirror real apps as much as possible.
| /// This is inc ase the `onAppear` callback triggers a re-render, | ||
| /// we don't hit our recurisve update precondition. |
There was a problem hiding this comment.
I dunno, this makes me nervous. If we allow this then you can get into weird scenarios where there's a render pass with callbacks, and it forces another layout which also has callbacks, and now they're out of order — the recursive render gets to complete its callbacks before the first one. In the worst case you could kick off a loop that never allows the render to complete.
Fundamentally I'm not sure there's a strong justification for anyone needing to force an explicit re-render during onAppear. Why did the element need to change as a result of appearing, instead of just using that element to begin with? Why did it need to change immediately and synchronously? Seems like there are some holes in this story.
Co-authored-by: Robert MacEachern <[email protected]>
|
Once again, annoyed about the extension thing |
|
|
||
| var temporaryWindow: UIWindow? = nil | ||
|
|
||
| func rootViewController() -> UIViewController { |
There was a problem hiding this comment.
Hmm, are you sure this is necessary? The existing lifecycle tests work fine without an app host.
| @@ -1,16 +1,16 @@ | |||
| import UIKit | |||
|
|
|||
| /// View which does not personally receive touches but allows its subviews to receive touches. | |||
| final class PassthroughView: UIView { | |||
| @_spi(BlueprintPassthroughView) public final class PassthroughView: UIView { | |||
There was a problem hiding this comment.
Can you pull out these unrelated changes?
There was a problem hiding this comment.
Not unrelated; moving LifecycleObserver, which depends on PassthroughView, means we need to expose this.
There was a problem hiding this comment.
(Also sorry, to be clear, one of the reasons I moved this was to get this running under our cocoapods tests – which right now does not run any of the CommonControls tests...)
This fixes an issue where folks would in some cases have to
Dispatch.asyncthe work they perform in theironAppear(or `onDisappear) blocks, because the lifecycle callbacks were triggered inside the update. They're not really parts of the render semantically (they more mean "the layout is done, now the element is visible), so this seems more correct.To properly test this, I had to move some stuff around:
BlueprintUI, since it's not really a "control".PassthroughViewand moved it intoBlueprintUIas well.