[ASViewController] Support optional node #3021
Conversation
- If a node isn't provided by developers via -initWithNode:, a default one will be created and used internally. - This allows developers to use ASViewController like a normal UIViewController and as a base class for all view controllers among which some use a node hierarchy and some don't.
- If its node isn't provided by users, don't replace the view controller's view with the default node's view because it might be loaded from a nib. - Init a vanilla ASDisplayNode if a node isn't provided.
AsyncDisplayKit/ASViewController.h
Outdated
| * | ||
| * @param node An ASDisplayNode which will provide the root view (self.view) | ||
| * @return An ASViewController instance whose root view will be backed by the provided ASDisplayNode. | ||
| * | ||
| * @see ASVisibilityDepth | ||
| */ | ||
| - (instancetype)initWithNode:(DisplayNodeType)node NS_DESIGNATED_INITIALIZER; | ||
| - (instancetype)initWithNode:(DisplayNodeType)node; | ||
|
|
||
| /** | ||
| * @return node Returns the ASDisplayNode which provides the backing view to the view controller. |
There was a problem hiding this comment.
We should mark the _node property nullable instead of using a dummy node. This will be breaking API for Swift, but it's cleaner and avoids the need for _providedNode.
There was a problem hiding this comment.
I'm not sold on having a nil node within the view controller. If we would go ahead with that, we always would have to check for if the node is nil on every call within ASViewController and also all subclasses need to consider that (Swift needs to unwrap). I know you can call methods on nil within Objective-C ...
We will have an overhead with having around a temporary node that's true ...
There was a problem hiding this comment.
I'm not concerned with overhead – it's simply incorrect to have a node around that you never touch or add to the window hierarchy (because we wrap it in this _didProvideNode flag). That's not any safer than having a nil node.
| if (!_nodeProvided) { return; } | ||
|
|
||
| ASDisplayNodeAssertTrue(!_node.layerBacked); | ||
|
|
||
| UIView *view = self.view; | ||
| CGRect frame = view.frame; | ||
| UIViewAutoresizing autoresizingMask = view.autoresizingMask; |
There was a problem hiding this comment.
So if you call -setNode: after the view is loaded, won't we be in chaos? self.node will get updated, but the node won't get hosted in the view hierarchy.
There was a problem hiding this comment.
The node property is currently declared as readonly. It should be not possible to set a node after a view controller is initialized.
There was a problem hiding this comment.
Ah OK I had assumed this diff made it readwrite to appease the community members who wanted to use nodes but didn't want to use initWithNode:. Cool.
There was a problem hiding this comment.
(Mirrored from our discussion on Slack) The move here is to use an implicitly-unwrapped optional for the node property. If they don't give us one, it's nil. If they unwrap let node: ASDisplayNode = vc.node in Swift (if they assume an ASViewController has a node) they get a crash. These crashes should be pretty easy to debug, and very easy to prevent (guard let node = vc.node else { assertionFailure("Expected vc to have a node"); return ; }).
This way everyone can tell whether an ASViewController has a node or not, but Swift users won't have to constantly unwrap their node properties manually.
|
Pushed a new change that changes the nil node behavior based on @Adlai-Holler and my Slack conversation. It also changes the usage of mixed between the getter |
|
Thanks @Adlai-Holler and @nguyenhuy for working together on that. |
Thanks to @nguyenhuy for the initial work! Unfortunately there were a couple of bigger changes since the last commit of the initial PR so I rebased and also cleaned it up a bit. Will replace #2810
This PR makes ASViewController's node optional. This allows developers to use ASViewController like a normal UIViewController. More importantly, it can be used as a base class for view controllers among which some use a node hierarchy and some don't.
Resolve #2760.