-
Notifications
You must be signed in to change notification settings - Fork 300
Embedded components #2193
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
base: master
Are you sure you want to change the base?
Embedded components #2193
Conversation
maragues-stripe
left a comment
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.
I focused on the Android part
android/src/main/java/com/reactnativestripesdk/NavigationBarManager.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/reactnativestripesdk/NavigationBarView.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/reactnativestripesdk/NavigationBarView.kt
Outdated
Show resolved
Hide resolved
| val uri = android.net.Uri.parse(url) | ||
| val builder = | ||
| androidx.browser.customtabs.CustomTabsIntent | ||
| .Builder() | ||
|
|
||
| // Set toolbar color for better UX | ||
| builder.setShowTitle(true) | ||
| builder.setUrlBarHidingEnabled(true) | ||
|
|
||
| val customTabsIntent = builder.build() |
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.
nit: You can probably move this outside of runOnUiThread
| if (manifestFile.exists()) { | ||
| def manifest = manifestFile.text | ||
|
|
||
| // Add launchMode="singleTask" |
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.
MainActivity with launchMode=singleTask will destroy all activities that previously existed on top of it.
Is that what we want?
In any case, can't we define a standard AndroidManifest.xml file, instead of doing this?
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.
launchMode=singleTask is necessary for deep linking with React Navigation:
https://reactnavigation.org/docs/deep-linking/#setup-on-android
And deep linking is necessary for using "secure webviews" (implemented with chrome tabs on Android) to return back to the app.
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.
And we can't use AndroidManifest.xml file, instead of modifying the manifest in the build.gradle?
Co-authored-by: maragues-stripe <[email protected]>
Resolved conflicts in: - android/build.gradle: Used master version with stripeVersion variable - ios/StripeSdkImpl.swift: Kept both authentication session properties and proper spacing - example/package.json: Used master versions (React Native 0.81.0, React 19.1.0) - example/ios/Podfile.lock: Used master version - example/yarn.lock: Used master version - yarn.lock: Used master version 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Committed-By-Agent: claude
SwiftLint auto-formatted iOS files after merge. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Committed-By-Agent: claude
Fixed three compilation errors: 1. NavigationBarView.kt: Use UIManagerHelper.getSurfaceId() instead of direct surfaceId access 2. NavigationBarView.kt: Cast context to ThemedReactContext for proper type 3. StripeSdkModule.kt: Add missing androidx.browser.customtabs.CustomTabsIntent import 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Committed-By-Agent: claude
Added androidx.browser:browser:1.8.0 as an implementation dependency to support the openAuthenticatedWebView functionality that uses CustomTabsIntent for Android. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Committed-By-Agent: claude
cttsai-stripe
left a comment
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.
Question: this API is not ready for GA yet. How do we prevent users to think it's production ready? Should I add an unstable_ prefix or a PrivatePreview suffix to the API or something like that?
We have standard annotations for native Android and iOS but do not set up one for RN yet. https://trailhead.corp.stripe.com/docs/mobile-sdk/payments/releasing-non-ga-apis-in-the-mobile-sdks.
I'll discuss with our team to see how we should deal with private preview on RN.
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.
nit: Could we add tests to prevent regression in a follow-up PRs? I think unit tests and e2e tests would be helpful.
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.
Sure. Although it's hard to test these things because they depend on an external service I can add tests in a follow-up PR.
Also, I'm building an example app focused on embedded components that uses a different backend that allows testing multiple merchants in this other PR. It's for manual testing but it covers everything.
| callbacks?.[functionName]?.(value); | ||
| } | ||
| } else if (message.type === 'openAuthenticatedWebView') { | ||
| const { url, id } = message.data as { id: string; url: string }; |
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.
Should we validate the url?
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.
That can be a good sanity check. Done here.
| useEffect(() => { | ||
| const subscription = AppState.addEventListener('change', (nextAppState) => { | ||
| if ( | ||
| appState.current.match(/inactive|background/) && | ||
| nextAppState === 'active' | ||
| ) { | ||
| if (pendingAuthWebViewPromise.current) { | ||
| const { id, callback } = pendingAuthWebViewPromise.current; | ||
| pendingAuthWebViewPromise.current = null; | ||
| callback(id, null); | ||
| } | ||
| } | ||
|
|
||
| appState.current = nextAppState; | ||
| }); | ||
|
|
||
| return () => { | ||
| subscription.remove(); | ||
| }; | ||
| }, []); |
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.
How does it work? Some opinions from claude:
- Race condition: AppState change might fire before deep link listener captures URL
- Always resolves with null: Doesn't actually pass the deep link URL to the callback
- No timeout: Promise could hang indefinitely if deep link never arrives
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.
Also:
The AppState subscription in useEffect is removed on cleanup, but:
- If EmbeddedComponent unmounts during auth flow, pendingAuthWebViewPromise.current is orphaned
- No cleanup of authenticationSession in iOS on unmount
Recommendation: Add cleanup in useEffect return:
subscription.remove();
// Cancel pending auth sessions
if (Platform.OS === 'ios') {
NativeStripeSdk.cancelAuthenticatedWebView?.();
}
pendingAuthWebViewPromise.current = null;
};
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.
Feel free to ignore comments from claude if those do not make sense.
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.
Cleaning up the promise is a good idea. Done here 👍
| const DEVELOPMENT_URL = | ||
| Platform.OS === 'android' ? 'http://10.0.2.2:3001' : 'http://localhost:3001'; | ||
| const PRODUCTION_URL = 'https://connect-js.stripe.com'; | ||
| const BASE_URL = DEVELOPMENT_MODE ? DEVELOPMENT_URL : PRODUCTION_URL; |
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 like DEVELOPMENT_MODE is always false
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.
Yep, this can be set to true while developing to switch to using a local server for the webview URLs and for enabling the debug mode in the webview.
Happy to leave a comment or make this configurable in a different way.
Removed the New Architecture delegate pattern from NavigationBarManager to make it compatible with both Old and New Architecture builds. The delegate pattern requires codegen-generated classes that are only available in New Architecture. Since NavigationBar is a simple component with only one prop, it doesn't need the delegate pattern. Fixes the Old Architecture build error: - Unresolved reference 'NavigationBarManagerDelegate' - Unresolved reference 'NavigationBarManagerInterface' 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Committed-By-Agent: claude
|
@cttsai-stripe thanks for the review!! 🙏
Cool 👍 Thanks again! 🙇 |
Summary
This PR adds support for Connect embedded components. In particular it adds support for 3 components:
New components can easily be added.
Most of the work is done using just React Native but I need to use native code for a couple of things:
Question: this API is not ready for GA yet. How do we prevent users to think it's production ready? Should I add an
unstable_prefix or aPrivatePreviewsuffix to the API or something like that?Motivation
https://docs.google.com/document/d/1tEm_mtsbHgbyhBoctYGLjmmLth0nI4b24osTIxN8_gM/edit?usp=sharing
Testing
I updated the backend to have an additional endpoint to return a client secret for the embedded components and added 3 screens to the example app.
Documentation
Select one: