Add extension points for global node behaviors, potentially replacing interface state delegates

[Adlai-Holler]

right at the outset:

• Causes no compiler warnings
• Does nothing unsafe or undocumented

Node controllers were added as a way for clients to customize node behavior globally. If you subclass ASTextNode and ASImageNode, you have to duplicate any common logic you want to add.

Node controllers however introduce additional complexity and duplication – if ASNetworkImageNode can fire off network requests, why does it need a controller? Nodes are in fact a sort of tiny view controller and we should stick with the original abstraction instead of adding a different one in.

This diff does something pretty strange, but I think useful. It modifies the node API to (1) offer a public ivar to ASDisplayNode and (2) guarantee that certain ASDisplayNode+Subclass category methods e.g. interface state methods are not implemented by the base class, and thus can be overridden safely in categories. To make it safe, we put in empty stubs for methods that they don't choose to implement.

I modified CatDealsCollectionView to demonstrate how this extension point could be used to add visibility logging events at the application level.

I propose that this API informally deprecates the Beta ASNodeController and the interfaceStateDelegates APIs. Those systems will be removed.

[Adlai-Holler]

cc #1227

[appleguy]

@Adlai-Holler the rationale seems strong; Node Controllers were indeed added to allow global extension to ASDisplayNode subclasses by non-Texture / app-specific users. Indeed we have also found difficulties with Node Controllers as most infrastructure operates on e.g. ASCellNode specifically.

For some use cases it may still be required to have this state tracking done outside of the ASDisplayNode instance, if there is the possibility of that instance itself being swapped out. This introduces the potential complication of a layer we don't want to be in the layout tree, but should have jurisdiction over node children (e.g. like a component). If this approach allows for that, then we should be good!

[Adlai-Holler]

@appleguy Thanks for reviewing this! This approach is agnostic toward what you mentioned – if the client ever wanted to carry state information between nodes when the nodes are swapped out, it is straightforward and easy to do.

[Adlai-Holler]

I'm curious to hear @maicki's take as well as @wiseoldduck and @wsdwsd0829 if they have time.

[wiseoldduck]

I think if we can accomplish what we want via the delegates, that would be better from an OOD standpoint. The problems you describe & we have encountered are typical to a customize-through-subclasses approach and are why customization-via-composition (the pattern which the delegates approach attempts) is generally considered better than customization-via-inheritance.

Categories help some but require the clients to understand the state of the node including locking, which seems less friendly than figuring out how to call the delegates from a consistent state where API re-entry is safe.

[Adlai-Holler]

OK I changed the names to baseDidInit and baseWillDealloc to be I think very clear. @maicki @wiseoldduck please review.

[wiseoldduck]

Well I am a lot more friendly to this now that I have seen what a $@#%*&(^ it will be to make the delegate & override methods safe for re-entry (always unlocked)

[Adlai-Holler]

We still should not call out with the lock held unless we document that for specific cases 😭

[nguyenhuy]

I think it’s ok to merge this diff as is. However, I’m a bit concerned about the proposal to deprecate and (eventually) remove interface state delegates because of 2 reasons:

• The arguments that node controllers introduce additional complexity and that we should stick with node being a thin view controller abstraction are strong to me. However, I’m not sure if the arguments are equally applicable to interface state delegates?
• Providing hooks/extensions via delegates is a concept familiar to people coming from UIKit world. On top of that, there are certain use cases that the delegate approach works great and is simpler than the new context approach. For example, one can have an impression tracker that cares about a subset of nodes over a short period of time (visible nodes during app cold start, for example). In this case, the tracker is a separate entity -- detached from the node tree or any other tree -- and isn’t interested in extending nodes on a global scale. As such, I believe it’s beneficial to provide both approaches and let users decide on which one works best for them in a particular use case.

[garrettmoon]

I want to echo Huy that removing the interface state tracking in a follow up PR would cause us a bit of a burden and I think it's useful.

[garrettmoon]

Additionally something to think through:

If someone decides to make a framework (let's say SuperAwesomeGridNode) that implements one of these categories, they're going to have to know that they need to explicitly warn their users not to also implement the category. And potentially create a different method that their category calls that's similar to this.

So if SuperAwesomeGridNode (ASDisplayNode) implements didEnterPreloadState they would have to document that they've done so, and implement another method `SAGNDidEnterPreloadState that they would have to stub out and call so clients could still hook in.

Long story short if we're going to hack this it should be documented with an @warning at the very least…

[Adlai-Holler]

Thats true. I think its unlikely that any framework devs would be crazy enough to do that but it IS a vulnerability of this approach.

…

On Feb 14, 2019, at 12:58 PM, Garrett Moon ***@***.***> wrote: Additionally something to think through: If someone decides to make a framework (let's say SuperAwesomeGridNode) that implements one of these categories, they're going to have to know that they need to explicitly warn their users not to also implement the category. And potentially create a different method that their category calls that's similar to this. So if SuperAwesomeGridNode (ASDisplayNode) implements didEnterPreloadState they would have to document that they've done so, and implement another method `SAGNDidEnterPreloadState that they would have to stub out and call so clients could still hook in. Long story short if we're going to hack this it should be documented with an @warning at the very least… — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[Adlai-Holler]

Thanks for the reviews guys. We'll keep interface state delegates around and land this to start removing reliance on node controllers. I'm pretty stoked about this because it's been a long-standing limitation of the framework!