[Init] Add executor providers to RCTBridge's init method

[ide]

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

[tadeuzagallo]

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 Nil param.

I also think you should assign it to _globalExecutorClass, otherwise your executor will get erased when you try debugging your app.

[ide]

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.

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 Nil param.

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 also think you should assign it to _globalExecutorClass, otherwise your executor will get erased when you try debugging your app.

I'll fix that, thanks for the catch. I am thinking to add another ivar though (maybe _debugExecutorClass?), since it might be surprising that the _globalExecutorClass is set, especially if you have two RCTBridge objects with different executor classes for example.

[tadeuzagallo]

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.

Sure, I know, I just meant it's really not used by us.

I can do this -- are there are a lot of places in the FB codebase that create RCTBridge objects though?

There are quite some places, because moduleProvider is still used and falls into the same case, if you need it, you have to create the bridge.

About the ivar, _globalExecutorClass doesn't seem right, it was more about recycling it, because I don't think it's ever used anymore. But might be better to get rid of it, if that's the case, and add a better suited ivar. Although I can't ever imagine an app, with two instances of the bridge, at the same time, running different executors, that shouldn't be a reason to not make it better. :)

[ide]

• Added debugExecutorClass
• Added convenience initializer that passes in the default classes (RCTContextExecutor and RCTWebSocketExecutor)
• Removed _globalExecutorClass

[ide]

Thinking about this some more, I think executorClass and debugExecutorClass should be replaced by executorBlock and debugExecutorBlock. This faciliates using the V8 debugger on the phone:

debugExecutorBlock: ^{
return [[RCTWebSocketExecutor alloc] initWithURL:proxyURLWithMacBookHost];
}

[tadeuzagallo]

Hey, sorry for taking so long...

I don't know if you have seen but I brought back the RCTWebViewExecutor, so usingDebugExecutor won't work... Are you actually using custom debug executors as well? If so we'll probably need to make the RCTDevMenu aware of it to enable this option.

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

[ide]

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.

[ide]

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.

[mfikes]

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 JSContext being used by React Native. I previously had success when RCTRootView allowed you to specify your own RCTContextExecutor (I was using Bentho). @ide 's RCTJavaScriptExecutorSource proposal sounds flexible enough.

[tadeuzagallo]

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.

[mfikes]

@tadeuzagallo FWIW, we've have the ClojureScript REPL / Om working for now with latest React Native by setting the executorClass between alloc and initWithBundleURL:moduleProvider:launchOptions: (a very nice suggestion by @jolby ):

RCTBridge *bridge = [RCTBridge alloc];
bridge.executorClass = [BTHContextExecutor class];
bridge = [bridge initWithBundleURL:jsCodeLocation
moduleProvider:nillaunchOptions:launchOptions];

[brentvatne]

ping @tadeuzagallo

[tadeuzagallo]

Although I like this better than the initial proposal,

I really don't think this is where we are going.

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 RCTDevMenu, with this proposal you can create a new type of executor, but it'll never show up in the menu. It also introduces a lot of manual work in adding an executor, where it should be same as adding a module.