-
Notifications
You must be signed in to change notification settings - Fork 271
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
Support events in Financial Connections #1828
Conversation
1aac014
to
53df471
Compare
if (stripeSdkModule != null && stripeSdkModule.eventListenerCount > 0) { | ||
FinancialConnections.setEventListener { event -> | ||
val params = mapFromFinancialConnectionsEvent(event) | ||
stripeSdkModule.sendEvent(context, "onFinancialConnectionsEvent", params) | ||
} | ||
} |
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.
This is a “best guess” approach, since I don’t see a way for us to reliably track if a listener is registered for onFinancialConnectionsEvent
.
53df471
to
35f09f0
Compare
@@ -84,7 +84,7 @@ type NativeStripeSdkType = { | |||
collectBankAccount( | |||
isPaymentIntent: boolean, | |||
clientSecret: string, | |||
params: PaymentMethod.CollectBankAccountParams | |||
params: Omit<PaymentMethod.CollectBankAccountParams, 'onEvent'> |
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.
No need to pass onEvent
around.
f0a1c5d
to
f1400cc
Compare
android/src/main/java/com/reactnativestripesdk/CollectBankAccountLauncherFragment.kt
Show resolved
Hide resolved
super.onCreate(savedInstanceState) | ||
|
||
val stripeSdkModule: StripeSdkModule? = context.getNativeModule(StripeSdkModule::class.java) | ||
if (stripeSdkModule != null && stripeSdkModule.eventListenerCount > 0) { |
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.
same here, do something like this:
stripe-react-native/android/src/main/java/com/reactnativestripesdk/PaymentSheetFragment.kt
Lines 143 to 148 in 447804f
if (stripeSdkModule == null || stripeSdkModule.eventListenerCount == 0) { | |
return@CreateIntentCallback CreateIntentResult.Failure( | |
cause = Exception("Tried to call confirmHandler, but no callback was found. Please file an issue: https://github.com/stripe/stripe-react-native/issues"), | |
displayMessage = "An unexpected error occurred" | |
) | |
} |
src/functions.ts
Outdated
const subscription = | ||
params.onEvent && | ||
eventEmitter.addListener('onFinancialConnectionsEvent', params.onEvent); |
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.
we have an existing paradigm for this in this file:
stripe-react-native/src/functions.ts
Line 357 in f1400cc
let confirmHandlerCallback: EmitterSubscription | null = null; |
I think it should be fine with this setup overall, but i'd rather keep it consistent with the other subscriptions we have
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.
Gotcha, will take a stab at it!
ab5f22a
to
a06c6f1
Compare
a06c6f1
to
be9f011
Compare
f67b577
to
34cc267
Compare
be9f011
to
8d8e12f
Compare
Use `financialConnectionsEventListener` instead of local subscriptions
8d8e12f
to
89e93ec
Compare
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.
Looks good! just one comment on the changelog but otherwise 🚢
CHANGELOG.md
Outdated
|
||
**Features** | ||
|
||
- Added ability to pass an `onEvent` listener to Financial Connections methods via a `params` argument. |
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.
let's put a list of which methods, just to be explicit
Summary
This pull request adds support for passing an
onEvent
handler to Financial Connections methods, similar to the native Android and iOS SDKs.Motivation
MOBILE_APIREVIEW-117
Testing
Documentation
Select one: