[feat]: merge ViewEnvironmentPropagatingObject into ViewEnvironmentPropagating #218
Conversation
n8chur
changed the title
n8chur
force-pushed
the
westin/viewenvironment-object-nodes-only
branch
2 times, most recently
from
965f764 to
4202cc9
Compare
| // Avoid updating the descendant if this is the case. | ||
| guard descendant.environmentAncestor != nil else { | ||
| continue | ||
| if descendant.environmentAncestor === self { |
There was a problem hiding this comment.
Outside of the more mechanical refactor to merge ViewEnvironmentPropagatingObject into ViewEnvironmentPropagating, this is the most substantial change in the PR.
This change ensures cases like ViewControllerDescription environmentAncestor overrides will prevent invalidations from other nodes who have this node set as a descendant.
n8chur
changed the title
| assert( | ||
| newValue == nil | ||
| || environmentAncestorOverride == nil, | ||
| "Attempting to set environment ancestor override when one is already set." |
There was a problem hiding this comment.
IMO it's a bit odd to require clearing a setter before you set it again. Thoughts?
There was a problem hiding this comment.
Yup, it's definitely a bit weird, but we had it in place to ensure folks aren't overriding an existing override without acknowledging that they may be overriding an existing override—this assertion has caught unexpected scenarios like this in the past.
Now that the framework has evolved a bit more, and I think we're more confident in the right shape for how to utilize these types, I don't feel as strongly that it's still necessary. I'm fine with removing it now if other folks agree!
There was a problem hiding this comment.
+1. Is it intended that we allow this to be nil'd out?
There was a problem hiding this comment.
I'm in favor of leaving it. If the propagation path gets messed up it can cause very subtle bugs downstream that are difficult to diagnose. I think the extra step to unwire existing overrides before wiring new ones is worth it to avoid that kind of mistake.
There was a problem hiding this comment.
OK, I'm leaning towards keeping it for now, but let me know if anyone feels strongly that this shouldn't require unwiring before overriding!
| assert( | ||
| newValue == nil | ||
| || environmentDescendantsOverride == nil, | ||
| "Attempting to set environment descendants override when one is already set." |
| assert( | ||
| newValue == nil | ||
| || environmentAncestorOverride == nil, | ||
| "Attempting to set environment ancestor override when one is already set." |
There was a problem hiding this comment.
+1. Is it intended that we allow this to be nil'd out?
| assert( | ||
| newValue == nil | ||
| || environmentAncestorOverride == nil, | ||
| "Attempting to set environment ancestor override when one is already set." |
There was a problem hiding this comment.
I'm in favor of leaving it. If the propagation path gets messed up it can cause very subtle bugs downstream that are difficult to diagnose. I think the extra step to unwire existing overrides before wiring new ones is worth it to avoid that kind of mistake.
| self, | ||
| &AssociatedKeys.needsUpdateObservers, | ||
| newValue, | ||
| .OBJC_ASSOCIATION_RETAIN |
There was a problem hiding this comment.
since this is UI code, should the nonatomic association be used? i assume this should never run concurrently
There was a problem hiding this comment.
(going to merge this PR but will follow up if @watt or others suggest a reason not to do this)
There was a problem hiding this comment.
Seems OK to me! I don't recall any reason it was done this way originally.
Overview
This PR is part of the larger effort to add support for automatic
ViewEnvironmentbridging betweenUIKitpropagation andWorkflowUIpropagation withinWorkflowUI.See the associated proposal for more information.
Merge
ViewEnvironmentPropagatingObjectintoViewEnvironmentPropagatingIn practice, all graphs attempting to adopt these protocols will be objects—value type systems (like
WorkflowUI's propagation ofViewEnvironmentthrough renderings) will not need to maintain nodes.My adding the restriction to
AnyObjecton the root protocol, we can perform pointer comparisons of nodes (see below for why that's useful).This consolidation of protocols likely also makes the system easier to understand.
Only invalidate descendants with a matching ancestor
Updates the descendant invalidation check such that a node will only invalidate descendants if the descendant node's ancestor is set to that node. This prevents invalidating nodes that have their propagation paths overridden (e.g.
ViewControllerDescription's custom ancestor node). This should improve performance by reducing unnecessary invalidations.Square Integration PRs
https://github.com/squareup/market/pull/6286
https://github.com/squareup/ios-register/pull/86301
Checklist