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

Expose TurboSession allowing use outside the module #254

Closed
wants to merge 1 commit into from

Conversation

Fryzu
Copy link

@Fryzu Fryzu commented Dec 21, 2022

Fixes: #245

Summary

Hello 👋 I am software engineer working for Software Mansion. Together with @pfeiffer we are working on React Native support for Turbo 🚀. Our goal is to add support for React Native hybrid apps on both iOS and Android for turbo enabled websites. Under the hood we use both Turbo Android and Turbo iOS. We have an almost ready version of our library, please check out our repo react-native-turbo-webview.

What's the problem?

iOS implementation with Turbo iOS was fairly easy, but the way Turbo Android depends on Android Navigation, makes the Android-implementation less straightforward.

Turbo Android doesn't leave much freedom for our implementation and It is impossible to use it without the Android Navigation which we can't use in the case of React Native. We are forced to use session class directly, sadly it is not easy as it is marked with internal visibility modifiers.

We solved this problem in forked repo, at the moment we are using a patched version of turbo-android for the PoC.

Removing internal modifiers makes it possible to use the package similarly to Turbo iOS and unifies the API. I do not see any reasons against the removal of modifiers but I am open to discussion over alternative solutions.

Changes

  • The PR removes several Kotlin internal visibility modifiers and makes TurboSession class accessible beyond module scope.
  • Adds isRunningInAndroidNavigation property (with default true) and changes one if statement condition. Does not change anything in the method when isRunningInAndroidNavigation is false so it won't affect non React-Native users.

@@ -53,6 +54,7 @@ class TurboSession internal constructor(
internal val httpRepository = TurboHttpRepository(activity.lifecycleScope)
internal val requestInterceptor = TurboWebViewRequestInterceptor(this)
internal val fileChooserDelegate = TurboFileChooserDelegate(this)
var isRunningInAndroidNavigation = true
Copy link
Author

Choose a reason for hiding this comment

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

Public property with default true. Set in my library to false to change behavior callback method behavior.

@@ -571,7 +573,7 @@ class TurboSession internal constructor(
private fun callback(action: (TurboSessionCallback) -> Unit) {
context.runOnUiThread {
currentVisit?.callback?.let { callback ->
if (callback.visitNavDestination().isActive) {
if (!isRunningInAndroidNavigation || callback.visitNavDestination().isActive) {
Copy link
Author

Choose a reason for hiding this comment

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

If statement mentioned in the PR description.

@Fryzu Fryzu changed the title Adds support for React Native Add support for React Native Dec 21, 2022
@Fryzu Fryzu changed the title Add support for React Native Expose TurboSession allowing use outside the module Dec 22, 2022
@dhh
Copy link
Member

dhh commented Dec 23, 2022

@jayohms can you have a look here?

@jayohms
Copy link
Collaborator

jayohms commented Feb 21, 2023

Thank you for the PR, however this isn't a suitable change for the Android library. Proper internal/public separation is important to maintain the library long-term. The Android library is quite deliberate in providing fully native navigation and wasn't designed to use the TurboSession independently.

While the iOS library may work for your needs, we have plans going forward for it to be more opinionated in providing out-of-the-box navigation and presentation between ViewControllers.

All the source code in turbo-android is available for your use to fork/copy and provide a good React Native experience — and since it sounds like you're handling the navigation aspects, you shouldn't need to bring too much over in your own library.

@jayohms jayohms closed this Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

React Native
3 participants