Fix bug where transitions would not complete if the presentation controller didn't implement the startWithContext method #45
Conversation
…roller didn't implement the startWithContext method.
| @@ -102,7 +102,8 @@ - (instancetype)initWithTransitions:(NSArray<NSObject<MDMTransition> *> *)origin | |||
|
|
|||
| _completionBlocks = [NSMutableArray array]; | |||
|
|
|||
| if (_presentationController) { | |||
| if (_presentationController | |||
| && [_presentationController respondsToSelector:@selector(startWithContext:)]) { | |||
There was a problem hiding this comment.
Does it make sense to call out that we are checking for compliance to MDMTransition rather than the specific method?
[_presentationController conformsToProtocol:@protocol(MDMTransition)]
There was a problem hiding this comment.
Checking for protocol conformity requires that the client explicitly conform to the protocol, while this check is a bit more lenient.
There was a problem hiding this comment.
I agree that it is more lenient, I'm not sure that is preferable. However, as this is a private component, we can save that spirited debate for a later date. 😁
There was a problem hiding this comment.
Checking whether the instance responds to the selector is the only safe guarantee we have that the method is implemented and won't crash if invoked.
For example, if a given API becomes optional in the future then a conformsToProtocol check would not only be unhelpful, but if we did not then also check respondsToSelector then it would also lead us to a crash.
There was a problem hiding this comment.
If your argument is against duck typing, however, then that is a separate discussion.
My personal belief is that a shared component library should be lenient on how Objective-C clients are expected to implement protocols and I've followed this practice through Nimbus, GoogleKit, MDC, and Material Motion. In other words, I believe that duck-type conformity is preferable over strict conformity for Objective-C code because it's more future-proof and lenient on casual conformers.
Aside: Swift brings its own opinions into this discussion because it is not possible to have optional methods in pure Swift protocols, so in Swift it's much easier to do strict conformity (obj as? MyType). As a result, duck typing is not often seen (and that's ok!).
There was a problem hiding this comment.
Different philosophies I think.
My feelings lean more toward, if you want to use Q you must conform to W.
I don't know that I've experienced to many protocols being made optional, usually just additions or complete deprecations.
Which leads me to my concern that MDMTransition would add a method, endWithContext: perhaps. We would then be checking for two methods. Additionally, by checking only for respondsToSelector, we aren't 'verifying' the parameter types of the selectors which conforms would do.
I think it comes down to the balance of ease of use for casual conformers -vs- strict(!) APIs.
So,...... pistols at dawn? 🔫 🌅 😄
|
This commit is blocking #44. |
| @@ -102,7 +102,8 @@ - (instancetype)initWithTransitions:(NSArray<NSObject<MDMTransition> *> *)origin | |||
|
|
|||
| _completionBlocks = [NSMutableArray array]; | |||
|
|
|||
| if (_presentationController) { | |||
| if (_presentationController | |||
| && [_presentationController respondsToSelector:@selector(startWithContext:)]) { | |||
There was a problem hiding this comment.
I agree that it is more lenient, I'm not sure that is preferable. However, as this is a private component, we can save that spirited debate for a later date. 😁
This also fixes our unit tests.