Skip to content
This repository has been archived by the owner on Feb 2, 2023. It is now read-only.

[ASViewController] Support optional node #2810

Conversation

nguyenhuy
Copy link
Contributor

@nguyenhuy nguyenhuy commented Dec 20, 2016

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.

@hannahmbanana
Copy link
Contributor

hannahmbanana commented Dec 20, 2016

This is really awesome Huy! I've heard that ASViewController's initWithNode: pattern is the most confusing aspect of ASDK.

@maicki
Copy link
Contributor

maicki commented Dec 20, 2016

@nguyenhuy After looking over this PR it's a really nice improvement idea, but I'm not pretty sure if we should introduce having to test if the _node is nil in almost every method. Especially going forward we would need to be careful to always consider that.

What about having something like a ASViewControllerNullNode that would act like a proxy and this would be used as root node for the ASViewController and has a reference to the view controller if no node is given? In that null node we would just not do anything I guess in case we call method's like layoutThatFits: but if the view is accessed we actually would return the view from the view controller. I would have to lookup in the ASViewController implementation what we exactly would need, but I think that would be a bit cleaner solution.

@nguyenhuy nguyenhuy force-pushed the 2760_ASViewController_nullable_node branch 3 times, most recently from 1667fb4 to 907f9a9 Compare December 21, 2016 17:36
@nguyenhuy
Copy link
Contributor Author

@maicki Thank you for the feedbacks. I've came up with a simpler fix. In case users don't provide ASViewController with a node (by using -initWithNibName:bundle: or -initWithCoder:, I create a default node that simply loads an UIView. And so the node is non-optional as it has always been. This introduces a small overhead but helps reduce complexity of ASViewController. The overhead might even encourage people to opt-in to a node backed hierarchy and take full advantage of ASViewController.

I tested this approach with ASDKgram and didn't find any problem. Given the fact that we currently don't even support -initWithNibName:bundle: or -initWithCoder:, this change shouldn't cause any new regression on existing codebases and thus it should be safe.

- 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.
@nguyenhuy nguyenhuy force-pushed the 2760_ASViewController_nullable_node branch from 907f9a9 to 510ce09 Compare December 21, 2016 17:46

_node = [[ASDisplayNode alloc] initWithViewBlock:^UIView * _Nonnull{
return [[UIView alloc] init];
}];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use simply [[ASDisplayNode alloc] init] here because using a view block results in a wrapper node, which has limited capacity compared to a node using _ASDisplayView.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Thanks. Fixing.

@nguyenhuy nguyenhuy force-pushed the 2760_ASViewController_nullable_node branch from 2912ed5 to 8969e1a Compare December 21, 2016 18:28
- 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.
@nguyenhuy nguyenhuy force-pushed the 2760_ASViewController_nullable_node branch from 8969e1a to 95cc041 Compare December 21, 2016 18:29
@hannahmbanana hannahmbanana added this to the 2.0.3 milestone Jan 17, 2017
@garrettmoon garrettmoon modified the milestones: 2.2, 2.0.3 Feb 9, 2017
@Adlai-Holler
Copy link
Contributor

It looks like this has been updated by @maicki in the linked PR. Closing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants