Make setJSEngineResolutionAlgorithm a public method

[SparshaSaha]

Summary:

setJSEngineResolutionAlgorithm is marked as private while it looks like it was intended to be public.
While the current implementation does not cause any issues as such, but in brownfield we will have the option to explicitly set the JS engine to hermes or jsc when initialising the ReactInstanceBuilder.

Right now, we look if the engine is set and if unset we look for the jsc engine. If jsc is not present we select hermes and this process unnecessary throws a warning.

Changelog:

[ANDROID][FIXED] - Changed the scope of setJSEngineResolutionAlgorithm to public from private. Brownfield apps should be able to setup the JSResolutionAlgorithm before hand.

Test Plan:

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,547,531	+0
android	hermes	armeabi-v7a	7,861,911	+0
android	hermes	x86	9,029,447	+0
android	hermes	x86_64	8,884,496	+0
android	jsc	arm64-v8a	9,167,632	+0
android	jsc	armeabi-v7a	8,358,032	+0
android	jsc	x86	9,224,304	+0
android	jsc	x86_64	9,482,162	+0

Base commit: a44d8a0
Branch: main

[facebook-github-bot]

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[cipolleschi]

Hi @SparshaSaha, thank for the PR. However, I don't think we should change the visibility of this method.

The way a JS engine is set is through the setJavaScriptExecutorFactory, not through the setJSEngineResolutionAlgorithm.

If you look at the code down there, we create a ReactInstance using the defaultExecutorFactory or the one passed by the user. If there is a custom executor factory, the resolutionAlgorithm is not even considered.
For the defaultFactory, we check what is set and we try to create the appropriate algorithm.

Making this public can create inconsistencies: what happens if the app passes a JSCExecutorFactory, but it set an HermesResolutionAlgorithm? By keeping the algorithm private, we can ensure consistency.

Does this make sense?

Also, there is another problem: when we build for Hermes, we don't have the JSC symbols in the build, so we cannot really allow to set different resolution algorithms.

[SparshaSaha]

I see @cipolleschi . Thank you for taking a look at it. It makes sense.

However, in that case, do you think we can remove this method: setJSEngineResolutionAlgorithm altogether?
I can see it's private but not used within this class.

As far as I understand, this condition will always be true right?

react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerBuilder.java

Line 365 in a44d8a0

if (jsEngineResolutionAlgorithm == null) {

If that's the case can we remove this altogether? The logger says, "You're not setting the JS Engine Resolution Algorithm. " + "We'll try to load JSC first, and if it fails we'll fallback to Hermes");
but there seems to be no existing way to set this variable.

[cipolleschi]

yeah, you are right. I reached out to @cortinico who has more context on how this should be used. IIRC, we had to keep that variable for backward compatibility with the internal infra, unfortunately. But let's wait for his input! ;)

[cortinico]

yeah, you are right. I reached out to @cortinico who has more context on how this should be used. IIRC, we had to keep that variable for backward compatibility with the internal infra, unfortunately. But let's wait for his input! ;)

Thanks for spotting this. You're right @SparshaSaha, we need to add a bit of logic as we're not properly populating the InstanceManager (that's why the method was unused). @javache took over this and is pushing a fix together with your change 👍

[facebook-github-bot]

@javache merged this pull request in cb376dd.