Skip to content
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

Card: Remove try/catch from Vault Flow #303

Merged
merged 12 commits into from
Dec 4, 2024
Merged

Conversation

sshropshire
Copy link
Collaborator

Summary of changes

  • This PR removes try/catch from CardClient.vault()
  • To do this, we've refactored UpdateSetupTokenResult to make it a sealed class with .Success and .Failure subclasses
  • Using "Result" types cleans up the codebase by providing compile time support for error handling
  • This is a follow up PR to Card: Remove try/catch from Approve Order Flow #302

Checklist

  • Added a changelog entry

Authors

List GitHub usernames for everyone who contributed to this pull request.

@sshropshire sshropshire requested a review from a team as a code owner December 3, 2024 20:06
@KunJeongPark
Copy link
Collaborator

KunJeongPark commented Dec 4, 2024

Are you going to do this for all the API calls? It's an interesting pattern. I'm thinking about this for iOS.
Not sure if it works with async/await pattern in iOS. It'd be weird, to have do catch block and non throwing function that returns Result type. It takes away advantage of error propagation in iOS async/await.

But I see that's similar to what you are doing here. You have try/catch block that returns a result type.

I think you had mentioned that you were going to remove listener pattern eventually.
So the merchant would receive the result type as well?

@sshropshire
Copy link
Collaborator Author

@KunJeongPark yes for Kotlin it's considered a best practice. Honestly I'm not sure this will be needed for iOS though. Swift does have checked exceptions i.e. when you call code that throws you have to surround it with do/catch in order for compilation to succeed. On Kotlin the compiler doesn't even warn us when we call throwing code without exception handling, and that's the primary reason we need a pattern like this on Android to make sure we have compiler enforcement.

val errorDescription = "Error loading resource with id $resId."
val ioError = PayPalSDKError(0, errorDescription, reason = e)
LoadRawResourceResult.Failure(ioError)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it throw other exceptions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question. I took a thorough tour of the docs and here are all the methods called:

According to the docs InputStream#read() will only throw if resAsBytes is null, and through Kotlin we have guarantees that it is non-null. All the other possible exceptions we've caught.

@sshropshire sshropshire force-pushed the card_vault_remove_try_catch branch from 5e57637 to ba94513 Compare December 4, 2024 20:27
@sshropshire sshropshire merged commit 41f3f56 into main Dec 4, 2024
6 checks passed
@sshropshire sshropshire deleted the card_vault_remove_try_catch branch December 4, 2024 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants