Remove Async keyword from PayPalNativeCheckoutClient.start function and fix tests #212
Conversation
| @@ -42,7 +42,7 @@ public class PayPalNativeCheckoutClient { | |||
| public func start( | |||
| request: PayPalNativeCheckoutRequest, | |||
| presentingViewController: UIViewController? = nil | |||
| ) async { | |||
| ) { | |||
There was a problem hiding this comment.
We will want to add a CHANGELOG entry for this since it's a public method
There was a problem hiding this comment.
That's a good call. Is this considered a breaking change in iOS?
There was a problem hiding this comment.
Yes this is a breaking change, if we made this change a merchant app would not compile as-is
There was a problem hiding this comment.
I would deprecate the old method until we're ready for V2
There was a problem hiding this comment.
Definitely yeah that makes sense 👍 .
There was a problem hiding this comment.
I don't think it's confusing to use the same method name for the async/non async method. This also means we won't have to go back and change the name to the one we actually want once we remove the deprecated method.
There was a problem hiding this comment.
It is pretty annoying that Xcode won't let you use the non-async start() function in a Task just because one exists with a duplicate name. That isn't the behavior of a method without a duplicate name.
Also this is the first time we're dealing with the fact that Swift doesn't support silencing deprecation warnings via Macros (example).
We could leave a NEXT_MAJOR_VERSION note to change this function signature later on when we're ready for v2? Or lean into how Swift handles this and refactor our demo.
There was a problem hiding this comment.
Steven's vote was for change in v2 since it is a breaking change.
And the issue that I ran into with the naming, merchant might run into also with duplicate names with deprecated method. We break out getting orderID in the demo app for testing purposes in other payment methods (I will make a ticket to do so for native payment module as well), but we have no guarantee of how merchant integration works.
There was a problem hiding this comment.
Cool, I'm down if we want to leave a NEXT_MAJOR_VERSION. We use that tag so that when time comes for doing a breaking major version release we have all of our TODOs related to it easily searchable
There was a problem hiding this comment.
Just noting that we can have the two separate async and non-async functions if we rename the new function as I did in earlier commits or just assume merchants will not run into issue of having non-async function in a Task block with another async function ( which I don't think we should )
|
Our repo docs also need to be updated for this change, and our dev docs as well |
Do we keep the documentation for deprecated functions here and in the dev docs? The dev docs needs to be another ticket, different repo. I'll hold on dev docs change until we agree on the new function name |
Great question. If a function is deprecated in our SDK, our docs should not be using it. The docs should be updated to showcase the newest, suggested way of integrating.
Agreed! Dev docs is another repo, so a separate ticket is a great call |
| @@ -43,6 +43,7 @@ public class PayPalNativeCheckoutClient { | |||
| request: PayPalNativeCheckoutRequest, | |||
| presentingViewController: UIViewController? = nil | |||
| ) async { | |||
| // NEXT_MAJOR_VERSION: - Change to non-async | |||
There was a problem hiding this comment.
I think we want to move this above /// Present PayPal Paysheet and start a PayPal transaction
There was a problem hiding this comment.
Addressed here: dd91cff
Summary of changes
the results are returned via delegate methods.
no warnings, errors found. There are warnings and compile errors in other modules separate from this PR,
so I made another ticket to address them.
-Reverted above changes with comment to remove Async for next major version.
Checklist
- [ ] Added a changelog entryAuthors