-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
[Init] Add executor providers to RCTBridge's init method #652
Conversation
When I started refactoring, it was on this very place, but when I finished it, there wasn't a single call site using it (like happens in your diff). If you have an use case for that, I don't see any problem in having it, but please, create another initializer so we don't need to go through all the codebase adding a I also think you should assign it to |
The call site is in my app's code. I create the RCTBridge with this new initializer method, and pass the bridge into the RCTRootView.
I can do this -- are there are a lot of places in the FB codebase that create RCTBridge objects though? (I was thinking most of the time the bridge would be created by RCTRootView except in a few custom cases.)
I'll fix that, thanks for the catch. I am thinking to add another ivar though (maybe |
Sure, I know, I just meant it's really not used by us.
There are quite some places, because About the ivar, |
|
Thinking about this some more, I think debugExecutorBlock: ^{
return [[RCTWebSocketExecutor alloc] initWithURL:proxyURLWithMacBookHost];
} |
Hey, sorry for taking so long... I don't know if you have seen but I brought back the And if you decide to keep it, I think we should fallback to the default ones on the new initialization method, instead of passing them in on the old method. That way you can just specify one or other when you create the bridge. Also if you could rebase please... I'll try to keep it faster so you don't have to rebase again |
The custom debug executor is useful for configuring the hostname of the V8 debugger proxy (e.g. debugging on phone). I'll look into updating RCTDevMenu. |
e6f1e9e
to
314d487
Compare
hey @tadeuzagallo, I got this working with RCTDevMenu in a simpler way without a separate debugExecutor. There's a simple interface called RCTJavaScriptExecutorSource that you can optionally pass to the RCTBridge constructor. The RCTDevMenu and the keyboard shortcut handlers tell the RCTJavaScriptExecutorSource to create different types of executors (UIWebView, WebSocket, etc) and reload the bridge. The code is backwards compatible so existing users of RCTRootView and RCTBridge should not have to update their code. |
f7b1462
to
ed5a204
Compare
I'd like to see this issue addressed. +1 from me. Background: I'm working to establish a ClojureScript REPL (Ambly) into the mix, so that we can develop React Native apps with things like Om, and all that is really needed is access to the underlying |
I'm sorry for not replying sooner, I've been trying to address that with @nicklockwood but we haven't found a good enough solution yet, but I really don't think this is where we are going. I'm currently doing a lot of refactoring on the bridge, and we are trying not to keep changing user facing APIs. On the mean time I tried to fallback to a previous workaround suggested by @ide (delaying the initialization for 1 frame), but due to some other changes it didn't really work out now, and generated some racing conditions that weren't worth fixing for a quick workaround. |
@tadeuzagallo FWIW, we've have the ClojureScript REPL / Om working for now with latest React Native by setting the RCTBridge *bridge = [RCTBridge alloc];
bridge.executorClass = [BTHContextExecutor class];
bridge = [bridge initWithBundleURL:jsCodeLocation
moduleProvider:nil
launchOptions:launchOptions]; |
204a30c
to
bede2fb
Compare
4aebc1b
to
9acec4f
Compare
fc2686b
to
66fedc6
Compare
If you construct an RCTBridge you may want to configure the executor. However the constructor synchronously calls `setUp` and initializes the executor. Instead, let the code that constructs the bridge specify an executor source, which controls how the executor is provided. This allows for customizing the web view executor with a UIWebView of your choice, or configuring the URL of the debugger proxy that the web socket executor connects to. Now that RCTRootView takes a bridge in one of its initializers, it is possible to create a bridge with a custom executor and then use that to set up a root view as well. Test Plan: Run the UIExplorer app and confirm I am able to connect to the plain JSContext via Safari. Hit Cmd-D, pick Chrome and see Chrome open a Tab. Hit Cmd-N and see that I can connect to a plain JSContext again. Shake the simulator and select "Enable Safari Debugging" and see that I can connect to the UIWebView from Safari. tl;dr the keyboard shortcuts and dev menu work as expected. Fixes facebook#288
ping @tadeuzagallo |
Although I like this better than the initial proposal,
The idea is to executor to be converted in to bridge modules, we just didn't stick to an API that will address all the concerns we have yet. Most of this concerns are not addressed by this PR like removing the manual work done in |
If you construct an RCTBridge you may want to configure the executor. However the constructor synchronously calls
setUp
and sets up the executor. Instead, let the code that constructs the bridge specify the executor and a debug executor up front. This allows for customizing the web view executor with a UIWebView of your choice, or configuring the URL of the debugger proxy that the web socket executor connects to.Now that RCTRootView takes a bridge in one of its initializers, it is possible to create a bridge with a custom executor and then use that to set up a root view.
Fixes #288 and supersedes #291