support offline mode #564
support offline mode #564
Conversation
ianlin-bbpos
requested review from
billfinn-stripe,
bric-stripe,
nazli-stripe and
chr-stripe
as code owners
| @@ -961,4 +997,33 @@ class StripeTerminalReactNative: RCTEventEmitter, DiscoveryDelegate, BluetoothRe | |||
| let result = Mappers.mapFromReaderDisplayMessage(displayMessage) | |||
| sendEvent(withName: ReactNativeConstants.REQUEST_READER_DISPLAY_MESSAGE.rawValue, body: ["result": result]) | |||
| } | |||
|
|
|||
| func terminal(_ terminal: Terminal, didChange offlineStatus: OfflineStatus) { | |||
There was a problem hiding this comment.
| func terminal(_ terminal: Terminal, didChange offlineStatus: OfflineStatus) { | |
| func terminal(_ terminal: Terminal, didChangeOfflineStatus offlineStatus: OfflineStatus) { |
android/src/main/java/com/stripeterminalreactnative/StripeTerminalReactNativeModule.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/stripeterminalreactnative/StripeTerminalReactNativeModule.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/stripeterminalreactnative/StripeTerminalReactNativeModule.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/stripeterminalreactnative/StripeTerminalReactNativeModule.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Generally looks good. I left a few comments, some are echoing @nazli-stripe's comments as well. Thanks for the big lift here!
android/src/main/java/com/stripeterminalreactnative/ReactNativeConstants.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/stripeterminalreactnative/StripeTerminalReactNativeModule.kt
Show resolved
Hide resolved
android/src/main/java/com/stripeterminalreactnative/StripeTerminalReactNativeModule.kt
Outdated
Show resolved
Hide resolved
| "prefer_online" -> OfflineBehavior.PREFER_ONLINE | ||
| "require_online" -> OfflineBehavior.REQUIRE_ONLINE | ||
| "force_offline" -> OfflineBehavior.FORCE_OFFLINE | ||
| else -> OfflineBehavior.PREFER_ONLINE |
There was a problem hiding this comment.
I realize we are following the same pattern we do elsewhere, but I think we should consider more strictly validating the input string parameters, rather than assuming a default value whenever the string doesn't match an expected value. Specifically, I think we should raise an exception here rather than defaulting to PREFER_ONLINE.
No change requested for this PR, but please consider making this change broadly throughout the RN code base in the future.
| @@ -138,6 +139,7 @@ export interface StripeTerminalSdkType { | |||
| setSimulatedCard(cardNumber: string): Promise<{ | |||
| error?: StripeError; | |||
| }>; | |||
| getOfflineStatus(): Promise<OfflinePaymentStatus>; | |||
There was a problem hiding this comment.
Ooc, is there a reason this returns a Promise instead of just an OfflinePaymentStatus? The values should be immediately available from the native SDKs, so I don't think we need to make this async.
There was a problem hiding this comment.
Thank a lot for your comments~. we have also considered using the synchronous method before. Later, during the debugging process, we saw the RN document mentioning the suggestion that we use the asynchronous method to avoid possible performance problems. Please kindly correct us if we understand something wrong.https://reactnative.dev/docs/native-modules-android?android-language=kotlin
There was a problem hiding this comment.
Ok, got it. I'm fine with leaving this as async and returning a promise. 👍
android/src/main/java/com/stripeterminalreactnative/listener/RNOfflineListener.kt
Show resolved
Hide resolved
android/src/main/java/com/stripeterminalreactnative/listener/RNOfflineListener.kt
Outdated
Show resolved
Hide resolved
nazli-stripe
force-pushed
the
bbpos/support-offline-mode
branch
from
2852529 to
43eaa36
Compare
Summary
Support offline mode
Motivation
To support users to create and process PaymentIntents while offline.
Testing
Documentation
Select one: