[fix]: update WorkflowHostingController on layout if ancestor hierarchy has changed

[n8chur]

Problem

When a WorkflowHostingController is added to a UIViewController hierarchy, we need to ensure that we re-render the Screen using an updated ViewEnvironment, since the environment from the ancestor path above that WorkflowHostingController could contain customizations that are not present in the WorkflowHostingController itself.

Currently, it is expected that consumers would call setNeedsEnvironmentUpdate() when adding a WorkflowHostingController as a child, but this is fragile and hard to enforce/validate, especially in legacy code.

Omitting this setNeedsEnvironmentUpdate() can cause issues where content would be rendered on screen using invalid ViewEnvironment values on the first rendering. On a later environment update, we'd likely see this environment reflect the correct values, but we want to avoid showing content with bad ViewEnvironment information whenever possible. An example of this issue can be seen in this Slack thread from a in issue reported in ios-register.

Solution

While consumers should still try to call setNeedsEnvironmentUpdate() whenever the environment changes (either through an update in value or an update in path), this PR attempts to catch mistakes and legacy usecases by tracking the ancestor tree node path when state updates and layouts occur, and if that path changes, we'll ensure an update occurs on the next layout pass.

Integration

I've smoke tested this change in ios-register and market and no changes are needed upstream to support this fix. Here are the integration PRs (with no real code changes) that show passing CI jobs:

ios-register: https://github.com/squareup/ios-register/pull/88056
market: https://github.com/squareup/market/pull/6547

[kyleve]

Clever!

[kyleve]

Should we not do this if the path is empty? Eg, trying to think if a layout subviews can/will happen after being removed from the hierarchy, causing an extra or invalid layout pass.

[n8chur]

The WorkflowHostingController could be become the root of a window where there would be no ancestor node and I think we'd still want to perform an update in that case.

[watt]

I think we might have a potential retain cycle here, if this is storing strong references to the entire VC hierarchy above it. Can we weak-box each ancestor on the path, or does that break our ability to reliably compare paths?

[kyleve]

It's not a reference since it uses ObjectIdentifier(node) right? Eg, just a pointer.

[nononoah]

That's also my understanding.

[n8chur]

I talked through this with @watt and did some experimentation locally and it does look like ObjectIdentifier just keeps a reference to the pointer value, not the object itself.

One interesting note that @watt pointed out from the documentation of ObjectIdentifier:

This unique identifier is only valid for comparisons during the lifetime of the instance.

Andrew was wondering if the pointer could be reused for a new instance and cause false positives on the equitability check.

This seems fairly unlikely to occur in practice here, but we could add a weak box for these objects to better track the lifetime if we think it's worth the effort.

[n8chur]

I've updated this to use a weak box and === comparisons: fa85e0f