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

Embedded Capacitor: Pass urlPath to Bridge #3405

Closed
wants to merge 2 commits into from
Closed

Conversation

fkirc
Copy link
Contributor

@fkirc fkirc commented Aug 10, 2020

For Embedded Capacitor, it is often necessary to have multiple Capacitor fragments with different routes.
For example, I have two embedded Capacitor fragments like this:
http://localhost/feature_1
http://localhost/feature_2

This PR passes the /feature_1 and /feature_2 to the target bridges.
It is somewhat similar to the following iOS-issue: #3106

My intended usage of this PR is as follows (although not the only possible usage):

import android.os.Bundle
import com.getcapacitor.BridgeFragment

class MyFeatureBridgeFragment : BridgeFragment() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        super.setUrlPath("/feature_1")
    }
}

Limitations and Further Work

Passing only the URL path might not be enough for all use cases of Embedded Capacitor.
For example, one might have different hosts instead of different URL paths.
Although this practice might be forbidden by app store guidelines, it might still be a valid use case:
https://remote_host_1.com
https://remote_host_2.com

More generally, I propose a system where the entire Capacitor-configuration can be dynamically modified/overriden by the native code.
In turn, this may or may not lead to the elimination of the native capacitor.config.json-files.
Nevertheless, I still submit this PR because I cannot estimate how long it would take to have a new configuration system.

@fkirc fkirc marked this pull request as draft August 10, 2020 15:25
@fkirc fkirc marked this pull request as ready for review August 10, 2020 15:47
@carlpoole
Copy link
Member

carlpoole commented Aug 10, 2020

This is currently possible by using the following constructor for a new bridge fragment:

public static BridgeFragment newInstance(String startDir)

where startDir is the directory containing the app web assets. I think in the example given this would be "feature_1"

Although this does not suit the use case where you use the BridgeFragment by extending and inflating via XML, as an example. The way this is currently written would allow you to call newInstance with a startDir and specify a path in onCreate and the Bridge would first load one then the other. It may be cleaner to streamline this.

@fkirc
Copy link
Contributor Author

fkirc commented Aug 10, 2020

I actually tried to modify startDir, but this is not exactly the thing that I want.
My goal is to only change the route for each fragment, while still keeping the same startDir and the same index.html for all fragments.

Imagine a typical React-app with react-router: I only want to build a single React-app with multiple routes, instead of building multiple React-apps.

In other words:
There is a huge difference between multiple routes in a single React-app and multiple React-apps in different physical directories.

@fkirc
Copy link
Contributor Author

fkirc commented Aug 10, 2020

One more note about the newInstance-method:

My solution supports both XML-inflation and fragment manager, since it is perfectly fine to subclass a fragment and then instantiate it with newInstance or similar.

Therefore, it might be a good idea to replace the newInstance-method with a simple setter.

In #3280, I also made the experience that static builder methods are less flexible than simple setter methods.

@carlpoole
Copy link
Member

Ah I see, so it sounds like there are a couple of use cases:

  • Web view in fragment 1 points to one app directory, and web view in fragment 2 points to a different app directory
  • Web view in fragment 1 points to /feature_1 route in a web app directory, and fragment 2 points to /feature_2 route in the same app directory.

Am I understanding that correctly? If so then two different settings do make sense. One for the app directory and one optional for a route in that web app (default /)?

Yeah, this reminded me of the plugin PR also :)

@fkirc
Copy link
Contributor Author

fkirc commented Aug 10, 2020

Yes this is correct, those are two independent functionalities.
startDir is well-suited if you have two completely different web-apps, whereas the urlPath provides different entry-points (routes) into the same web-app.

Other than that, native route configuration might be also useful if the native app uses Capacitor to present a domain-object like localhost/object/id, where id is dynamically determined by the native code.

Furthermore, I also want to use urlPath to pass query parameters from the native code like so:
http://localhost/?my_param=some_arg_from_native_code

This practice removes the necessity to fetch parameters with custom Capacitor plugins, which leads to a potential speedup during launch time of the web-app (and simplified code).

In theory, this PR could be entirely replaced with custom Capacitor plugins, but the launch-performance would be worse and it would be more cumbersome to write.

@fkirc
Copy link
Contributor Author

fkirc commented Aug 24, 2020

Any updates on this PR?
Do you have a concrete solution to pass a urlPath at launch time?
In the worst case, I might need to write my own BridgeFragment and my own Bridge.java, or perhaps hack something together with reflection.
Or perhaps go a slower route via ReactRouter and a custom capacitor plugin (which degenerates the launch time of the web app).

@carlpoole carlpoole mentioned this pull request Aug 28, 2020
@carlpoole
Copy link
Member

Hi Felix, sorry for delay in response. We think this is definitely a good addition to embedded Capacitor and know there is opportunity to improve the embedded use case. While trying to think of a way to incorporate this now in context of upcoming potentially breaking changes and cleanup, we felt it would be best to wait and include changes to address this use case as part of the overall upcoming improvements for embedded Capacitor in v3 for both mobile platforms.

Thanks for bringing attention to this and helping improve Capacitor.

@carlpoole carlpoole closed this Aug 28, 2020
@fkirc
Copy link
Contributor Author

fkirc commented Sep 9, 2020

I have to admit that I am a little bit frustrated about the current merge-situation.
I neither know when Capacitor-V3 will be ready; nor do I know whether Capacitor-V3 will support this use case out-of-the-box.

One more sidenote:
I spotted a bug in BridgeFragment that causes some config in capacitor.config.json to be silently ignored.
However, it does not seem helpful to file another pull request in the current merge-situation.

@imhoffd
Copy link
Contributor

imhoffd commented Sep 9, 2020

@fkirc It's never easy in transitions between major versions like this. Luckily, we are happily accepting bug fixes to the 2.x branch. A new 2.4.1 release went out today, actually. We'd be happy to look at the bug fix for BridgeFragment for 2.x. We'd prefer if new features were added to Capacitor 3, though.

For what it's worth, we don't plan on increasing the scope of Capacitor 3 beyond what this issue outlines, and we're making good progress.

@fkirc
Copy link
Contributor Author

fkirc commented Sep 9, 2020

Perhaps I am mistaken, but I thought that Capacitor 3.X is developed on the main-branch.
Therefore, isn't this also a PR for Capacitor 3.X?

@imhoffd
Copy link
Contributor

imhoffd commented Sep 9, 2020

Yeah, that's correct. I believe @carlpoole is saying that there's a larger suite of changes being planned for the Embedded Capacitor use case. Although it may be possible today in Capacitor 2.x, it isn't a focus or a documented feature. It will be for Capacitor 3.x, though.

@carlpoole
Copy link
Member

The proposed addition in this PR is great and we are going to include a way to specify the path in 3.x. In its current form the changes will conflict with plans we are exploring in refining the configuration and Embedded developer experience. This development is expected to begin in the coming month and we would rather approach this as one larger improvement to both native platforms than merge a change for Android now that will change again soon. Please do continue to submit bug reports/fixes and suggest features as we are always interested in improving Capacitor. The help is greatly appreciated.

@fkirc
Copy link
Contributor Author

fkirc commented Sep 13, 2020

I agree that an Android-only change is not a viable long-term plan.
Therefore, I would recommend to merge #1972 in order to have the exact same functionality for iOS. If #1972 is merged, then I could submit a small writeup on how to implement this use case for iOS.

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.

None yet

3 participants