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

Support events in Financial Connections #1828

Merged
merged 8 commits into from
Feb 21, 2025
Merged

Conversation

tillh-stripe
Copy link
Contributor

@tillh-stripe tillh-stripe commented Feb 7, 2025

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

  • I tested this manually
  • I added automated tests

Documentation

Select one:

  • I have added relevant documentation for my changes.
  • This PR does not result in any developer-facing changes.

@tillh-stripe tillh-stripe force-pushed the tillh/fc-events-support branch 3 times, most recently from 1aac014 to 53df471 Compare February 7, 2025 20:49
Comment on lines +41 to +46
if (stripeSdkModule != null && stripeSdkModule.eventListenerCount > 0) {
FinancialConnections.setEventListener { event ->
val params = mapFromFinancialConnectionsEvent(event)
stripeSdkModule.sendEvent(context, "onFinancialConnectionsEvent", params)
}
}
Copy link
Contributor Author

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.

@tillh-stripe tillh-stripe force-pushed the tillh/fc-events-support branch from 53df471 to 35f09f0 Compare February 7, 2025 20:56
@@ -84,7 +84,7 @@ type NativeStripeSdkType = {
collectBankAccount(
isPaymentIntent: boolean,
clientSecret: string,
params: PaymentMethod.CollectBankAccountParams
params: Omit<PaymentMethod.CollectBankAccountParams, 'onEvent'>
Copy link
Contributor Author

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.

@tillh-stripe tillh-stripe force-pushed the tillh/fc-events-support branch from f0a1c5d to f1400cc Compare February 10, 2025 16:50
super.onCreate(savedInstanceState)

val stripeSdkModule: StripeSdkModule? = context.getNativeModule(StripeSdkModule::class.java)
if (stripeSdkModule != null && stripeSdkModule.eventListenerCount > 0) {
Copy link
Collaborator

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:

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
Comment on lines 471 to 473
const subscription =
params.onEvent &&
eventEmitter.addListener('onFinancialConnectionsEvent', params.onEvent);
Copy link
Collaborator

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:

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

Copy link
Contributor Author

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!

@tillh-stripe tillh-stripe force-pushed the tillh/fc-events-support branch 2 times, most recently from ab5f22a to a06c6f1 Compare February 18, 2025 14:45
@tillh-stripe tillh-stripe changed the base branch from master to tillh/try-to-fix-tests February 18, 2025 14:45
@tillh-stripe tillh-stripe force-pushed the tillh/fc-events-support branch from a06c6f1 to be9f011 Compare February 18, 2025 14:53
@tillh-stripe tillh-stripe force-pushed the tillh/try-to-fix-tests branch from f67b577 to 34cc267 Compare February 18, 2025 14:55
@tillh-stripe tillh-stripe force-pushed the tillh/fc-events-support branch from be9f011 to 8d8e12f Compare February 18, 2025 14:56
Base automatically changed from tillh/try-to-fix-tests to master February 18, 2025 17:51
@tillh-stripe tillh-stripe force-pushed the tillh/fc-events-support branch from 8d8e12f to 89e93ec Compare February 18, 2025 17:52
@tillh-stripe tillh-stripe marked this pull request as ready for review February 18, 2025 20:36
@tillh-stripe tillh-stripe requested review from a team as code owners February 18, 2025 20:36
Copy link
Collaborator

@charliecruzan-stripe charliecruzan-stripe left a 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.
Copy link
Collaborator

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

@tillh-stripe tillh-stripe merged commit c318f98 into master Feb 21, 2025
6 checks passed
@tillh-stripe tillh-stripe deleted the tillh/fc-events-support branch February 21, 2025 17:19
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