[RFC][RNTester] Move Flipper init out of app code

[passy]

Summary

This is an attempt to minimise the amount of setup code that's needed to get Flipper up and running in an application. I wanted to showcase this first in RNTester, but I still need to confirm that this pattern would even work with the standard template as the setup there is a bit unusual and I'm unsure if debug flavours will work there as expected.

Anyway, before going into this, I'd like to discuss if this is helpful in striking a better balance between adding code to people's apps and customisability.

Changelog

[Android] [Changed] Move Flipper initialisation into ReactAndroid core

Test Plan

RNTester builds, Flipper connects.

[analysis-bot]

RNTester (Android/hermes/arm64-v8a): 3276800 bytes
RNTester (Android/hermes/armeabi-v7a): 3117056 bytes
RNTester (Android/hermes/x86): 3442688 bytes
RNTester (Android/hermes/x86_64): 3407872 bytes
RNTester (Android/jsc/arm64-v8a): 4446208 bytes
RNTester (Android/jsc/armeabi-v7a): 4268032 bytes
RNTester (Android/jsc/x86): 4358144 bytes
RNTester (Android/jsc/x86_64): 4646912 bytes

[safaiyeh]

I love this change. I had issues with manual Flipper set up before. Removing the friction for app devs is 😍😍

[safaiyeh]

Where is swipe refresh used?

[passy]

In React core itself. I wanted to have looked into this but haven't had a chance yet. Somehow the swipe layout no longer compiles now without explicitly depending on it here.

[rickhanlonii]

Yes this is great!

[passy]

Did some more testing and I think this isn't feasible for the template as it works right now. I don't have a full understanding of how the dynamic generation of the com.facebook.react:react-native:+ gradle module works, but from what I gather this is precompiled in a release flavour and doesn't include any classes that are part of debug builds.

Practically speaking, this means you'll get this error when trying to build a debug version of your app generated from the tempalate:

I see two options here:

1. Move this into the main flavour. This would require also depending on all Flipper dependencies for release builds. We could do some hackery and only "link" against them (compileOnly) but if someone accessed the helper incorrectly, they'd crash with a ClassNotFoundException.
2. Generate a separate debug artifact. In the android/app/build.gradle, we'd then have

debugImplementation "com.facebook.react:react-native-debug:+"  // From node_modules
releaseImplementation "com.facebook.react:react-native:+"  // From node_modules

or some similar naming scheme.

[janicduplessis]

We generate prebuilt archives on releases, its somewhat documented here https://github.com/facebook/react-native/wiki/Building-from-source#publish-your-own-version-of-react-native.

What about publishing this as a separate package on npm (or maybe as part of react-native-flipper)? It could be installed / linked in the default template.

[passy]

Separate npm artifact could work, but I'm not sure if it's worth the effort, because we'd have to duplicate a lot of code and setup. We'd need another Gradle project, set up the AAR generation, make sure this ends up in the right npm package and also ensure that this can be tested properly. Certainly a possibility, though!

[janicduplessis]

I think it could be like a regular native module package, it gets built from souce by apps so there is no need to setup aar generation. Thinking about it more what about just including the code in rn-flipper and adding it to the default project template?

[passy]

@janicduplessis Could you expand on this? What is rn-flipper in this context? You definitely have a way better overview of how this works, so I'm really thankful for your feedback here. :)

[janicduplessis]

https://github.com/facebook/flipper/tree/master/react-native/react-native-flipper I’m wondering if it would make sense to move all RN <=> Flipper integration to this package and include it by default in the template.

[passy]

Gotcha. I don't think it's a good place, to be honest. This is really just setup code that belongs in your application, ultimately. The reason for moving it out of the template is just so that the default setup looks a bit cleaner. But if somebody were to remove a plugin here, for instance, or change its configuration, they would still effectively need to copy the body of the method.

But specifically, the plugins being set up here are very specific to what React Native ships with. If you were to ever move away from Fresco, OkHttp or made an API change to how the reactInstanceManager works, we'd need to update something in the Flipper repo should not be affected by this.

The way it is set up right now with all the code being part of the template itself provides the maximum flexibility here and we're only considering moving it out because it is quite verbose and adds a bit of mental overhead, especially when you're new to this kind of development and just want to poke around in the generated code.

[janicduplessis]

@passy Makes sense, I'm curious how did you test this change? Maybe the error is just that the generated archive wasn't updated with the code changes you made? Running ./gradlew :ReactAndroid:installArchives --no-daemon should compile the new archives.

If that isn't the issue we could just make the class available in all builds. It is pretty small anyway, I don't think we do anything special already to remove other debug classes from release builds anyway (https://github.com/facebook/react-native/tree/master/ReactAndroid/src/main/java/com/facebook/react/devsupport). Hopefully proguard can remove those classes.

[passy]

If that isn't the issue we could just make the class available in all builds. It is pretty small anyway, I don't think we do anything special already to remove other debug classes from release builds anyway (https://github.com/facebook/react-native/tree/master/ReactAndroid/src/main/java/com/facebook/react/devsupport). Hopefully proguard can remove those classes.

The problem here is that we would then need to depend on the Flipper dependencies for the release builds which get transitively pulled in. Alternatively, we make them compileOnly (mentioned that as (1) in #28052 (comment)), but the user experience if something goes wrong is really poor.

Makes sense, I'm curious how did you test this change? Maybe the error is just that the generated archive wasn't updated with the code changes you made? Running ./gradlew :ReactAndroid:installArchives --no-daemon should compile the new archives.

Thanks, I'll give that a try.

[passy]

I used scripts/test-manual-e2e.sh for testing and also manually inspected the artifacts generated when building debug or release builds which confirmed what I thought would happen:

So I think the correct course of action here is to set up another build artifact that gets generated as part of the :ReactAndroid:installArchives step. Given the highly custom setup that RN uses, I'm not quite sure what the right approach for this is, though.

I saw that there's some logic for different "variants" in the top-level react.gradle, but I'm not sure how that's currently being used.

[passy]

@DanielZlotin I found your fingerprints on some of the logic in release.gradle. Do you have any context on how it might be possible to publish another aar with a debug flavour/variant?

[DanielZlotin]

Right, it's been a long time since I touched this, not up to date on any of this so I could be way off..
I remember wanting to publish 2 different RN artifact variants when added support for the "new" JSCore (with full i18n support and without, or something along those lines).
I think the "standard" gradle way is to publish 2 different artifacts, like you suggested, but I don't know if that's the direction the project's maintainers want to take.

These may be of help, to give some context:
#22263
#24276

[janicduplessis]

Maybe @dulmandakh can help here? My knowledge of android build system is not good enough to assist more in figuring out the best way to do this.

If this requires too much work I feel like it might just be better to keep the flipper integration as is for android.

[passy]

@DanielZlotin Thanks a lot for the context!

Still struggling a bit figuring out how to create a separate maven publication that then hooks into the debug configuration. Just grepping around on GitHub it appears to be possible to publish AARs with debug flavours but I could only find this set up in projects that are a lot more "vanilla" then this.

[rickhanlonii]

@passy I just noticed this is still open, should we merge or abandon it?

[passy]

@rickhanlonii It's probably too invasive right now. I don't have enough insight into how the internal build process works and so far people seemed to have been okay with the code we add to the app itself, so I'm gonna close this here.