-
Notifications
You must be signed in to change notification settings - Fork 294
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 cardAddChallengeRequested
to BTThreeDSecureRequest
#1122
Conversation
@@ -49,6 +49,7 @@ import BraintreeCore | |||
/// Non-Mastercard cardholders will fallback to a normal 3DS flow. | |||
public var dataOnlyRequested: Bool = false | |||
|
|||
// NEXT_MAJOR_VERSION remove cardAddChallenge in favor of cardAddChallengeRequested |
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.
Can we deprecate this param also and direct merchants to set cardAddChallengeRequested
instead?
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.
I had initially but apparently it blows up cocoapods unless you allow warnings (CI failure here) which I don't think we want merchants doing. 😞 It was removed here: 04c8c69
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.
😢 One of the cons for moving to Swift is that there is no macro for silencing deprecation warnings in-line. We already saw this limitation a bit when trying to deprecate the start()
method on PPCP (PR).
I guess we can only deprecate something if it isn't used anywhere in our source code, to avoid warnings to be triggered. There might be some (slightly wonky) workaround we can do in our implementation to get around this.
We could map the merchant's cardAddChallenge
(deprecated) setting to an internal property (non-deprecated) so that referencing it doesn't trigger a warning.
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.
Yeah, it's pretty interesting to me that these attributes are pretty limited overall without allowing warnings. It'd be nice to have a cleaner way to mark properties/methods to be removed in a future version that works with all package managers. What does work is this - just tested locally and pod lib lint --subspec=ThreeDSecure
passes:
#if !COCOAPODS
@available(*, deprecated, renamed: "cardAddChallengeRequested", message: "Use cardAddChallengeRequested. This property will be removed in our next major version.")
#endif
Not sure how we feel about only notifying SPM + Carthage merchants. Curious to hear what y'all think.
Summary of changes
cardAddChallengeRequested
toBTThreeDSecureRequest
cardAddChallenge
cardAddChallengeRequested
Checklist
Authors