-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add empty ASViewController initializer to facilitate subclassing #1740
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@christianselig Thanks for putting together this PR and summing up the issue nicely.
To ensure the stand in capability that the doc mentions (and I personally believe it's a good thing), I can see 2 solutions:
- Mark
-init
as a designated initializer, essentially what this PR does. - Remove
NS_DESIGNATED_INITIALIZER
flag of-initWithNode:
, essentially revert Add NS_DESIGNATED_INITIALIZER to ASViewController initWithNode: #1054.
The second approach allows ASViewController to be a true stand in for subclassing while the first one is more surgical.
Either way, the new implementation of -init
is needed because currently people can call it and the resulting instance wouldn't be initialized correctly.
Approving this PR but I'm interested in opinions from others as well.
It seems reasonable (to me) for both symbols to be annotated with
|
Looks like an |
@christianselig would you mind fixing up this Swift test?
Then we should be good to merge this. Thanks! |
@jparise I've seemingly since nuked the repo that this PR came from, but I recreated it, is there a way to link it to this specific PR or should I close this and create a new one? |
If you have Alternatively, something like this with vanilla
|
@jparise Not 100% sure I understand. I have the changes applied, but how do I then apply it against this PR specifically? Am I able to |
Ah, sorry, I misunderstood. I think it's probably easier to create a new PR. I don't know the git(hub) magic for this case. |
Closing in favor of #1754. |
Per Slack discussion with @nguyenhuy.
If you want to use
ASViewController
throughout your app as a generalizedUIViewController
stand in (as the docs mention) it's tricky (at least in Swift) when you want to take a non-node based normalUIViewController
and make it inherit fromASViewController
(again to perhaps have a consistent root subclass) because there's only one designated initializer inASViewController
,init(node:)
which does not accept nil values despite the docs indicating that should be okay.Currently the only course of action is to pass in a dummy node (e.g.:
ASViewController(node: ASDisplayNode())
). Other courses of action include unmarkinginit(node:)
as the designated initializer or letting it accept nil values, but neither of those are optimal, per @nguyhuy:A proposed solution would be to introduce a second designated "empty" initializer that would just use the underlying
UIViewController
, makenode
nil, and facilitate subclassing, which is what I did in this PR.Other context: