-
Notifications
You must be signed in to change notification settings - Fork 80
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
Soft-deprecate ViewRegistry #816
Conversation
Would love some input on this, however I'm out of town until Dec 19th, I will be responding to feedback then. |
@@ -25,6 +25,8 @@ import ReactiveSwift | |||
/// In order for the registry to handle a given screen type, a view factory | |||
/// must first be registered using `register(screenType:factory:)`, where the | |||
/// factory is a simple closure that is responsible for instantiating a view. | |||
/// | |||
/// Deprecated: Return the appropriate `ViewControllerDescription` from `Screen` instead | |||
public struct ViewRegistry { |
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.
Should this be annotated as deprecated instead of commented as deprecated?
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.
This triggers tons of warnings. Especially on all the Screen
initializers. I decided to just use a comment here, and use the annotations for the register
call.
@@ -35,7 +35,7 @@ open class ScreenViewController<ScreenType: Screen>: UIViewController { | |||
return ScreenType.self | |||
} | |||
|
|||
public required init(screen: ScreenType, viewRegistry: ViewRegistry) { | |||
public required init(screen: ScreenType, viewRegistry: ViewRegistry = ViewRegistry()) { |
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.
Why the default arg?
Signed-off-by: Dhaval Shreyas <dhaval@squareup.com>
94bc244
to
89138ac
Compare
Superseded by #974 |
This change landed here: #974 |
Provides a path for
ViewRegistry
migration without any breaking API changes.Individual
Screen
s can move from usingregister(screenViewControllerType:)
to providing aviewControllerDescription
.Initializers for both
ScreenViewController
andContainerViewController
have default values forViewRegistry
.This is an awkward intermediate step since we're not gaining any compile-time safety here and are introducing some dangerous side-effects like:
ViewControllerDescription
describedViewController
cannot build a newViewController
using theViewRegistry
, since it will not have the correctViewRegistry
on init.ViewControllerDescription
can only return implementations ofScreenViewController
, if not, we'll see a run-time error.This is the Migration strategy 1 as described in #815