De-duplicate building Spannable on Android

[cubuspl42]

Summary:

A first step in my work on react-native-community/discussions-and-proposals#695

De-duplicate the code for creating Spannable on Android. I'm planning to add quite serious new features to this module. This would be really hard with the current level of code duplication.

Changelog:

[INTERNAL] [CHANGED] - De-duplicate building Spannable on Android

Test Plan:

I tried to ensure that the refactored code is relatively easy to prove to be equivalent to the original duplicated one, but there's always a risk of a human mistake in this process. So far, I have been testing this by ensuring that nothing broke in the Text example section in RNTester.

[cubuspl42]

@NickGerleman I would be extremely grateful if you helped me with reviewing this or finding somebody to review this 😊

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	16,803,002	-14
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	20,190,113	-4
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 3d09b6f
Branch: main

[NickGerleman]

@mdvacca would you be able to take a look at this? It relates to removing duplication for Spannable generation.

IDK what our unit test coverage looks like here.

[cubuspl42]

@NickGerleman Thanks!

@mdvacca I would be grateful if you took a look at this! 🫶

Unfortunately, if I were to answer the question, it would be "not so good"; I assume that if such unit tests existed, they would be located in react-native/packages/react-native/ReactAndroid/src/androidTest, and I don't think anything there is focused on testing Spannable.

[cubuspl42]

@NickGerleman @mdvacca

I can keep bumping. Let me know if there's anything I could do to move this forward

[NickGerleman]

I've been checking with @mdvacca offline. I think he was sick last week, but mentioned he was planning to take a look and give some feedback. Thank you for keeping us honest here.

[cubuspl42]

@mdvacca It would be awesome if you could give it a quick look before the end of the week. I could start working on eventual changes before the next round.

[mdvacca]

Hi @cubuspl42, thanks for the PR and apologies for the slow response.
I will analyze in details and provide feedback tomorrow!

[mdvacca]

Hi @cubuspl42, thanks for working on this. It’s due for us to reduce the duplication of this code!

I wanted to share general feedback about the PR before getting into details:

• You are creating new public classes and interfaces. We recently created a new package called com.facebook.react.internal, where we are going to start moving public classes and interfaces that are part of the “core” but not part of the public API of React Native. I believe this is a good set of classes and interfaces that we can start including in there. Could you move these new classes and interfaces into “com.facebook.react.internal.view.text”?
• As part of the test plan you mention:

  I tried to ensure that the refactored code is relatively easy to prove to be equivalent to the original duplicated one, but there's always a risk of a human mistake in this process. So far, I have been testing this by ensuring that nothing broke in the Text example section in RNTester.

Even if we have some tests for Text I don’t think they are enough for us to feel super comfortable about not breaking anything in production. The approach we usually take for these kind of refactors is running A/B test. From the PR standpoint, running an A/B test involves creating a feature flag in ReactFeatureFlags class and then refactor the PR to make sure we can gate the new changes. Can you add a new feature flag in ReactFeatureFlags and refactor your code to be able to A/B this refactor?

• You created this new class called “TextLayoutUtils” that contains several static methods that “add things to spannables or build spannables”. How about refactoring this code to use a builder pattern?
• All new public classes, interfaces and method must be documented
• OPTIONAL: We are starting to move the React Native codebase to Kotlin. This is not a must, but if you are very familiar with Kotlin it would be great to use kotlin in this refactor. Again this is not a hard requirement.

Thanks!

[cubuspl42]

@mdvacca

I pushed a new revision, addressing most comments. What is left:

How about refactoring this code to use a builder pattern?

Most usages of the builder pattern I've encountered were a substitute for the data class Kotlin copy() method; this isn't exactly a case like that.

Would you provide a few lines of code that show how things would look after the refactor so I'm sure we're on the same page?

Can you add a new feature flag in ReactFeatureFlags and refactor your code to be able to A/B this refactor?

This is going to be tricky, but it is probably possible.

From the operational perspective, you're talking about A/B tests that Meta could run on the Facebook app and/or other Meta apps, do I get this right?

[cubuspl42]

@mdvacca Let me know what you think of the Kotlin "port" of the new bits. For example, I used a named object for utils, instead of exposing public functions, as for now some of the names were quite generic, like addText. This might change once we close the other topic about the builder pattern.

[cubuspl42]

@mdvacca While I'm trying to take a humorous approach, this is a serious, externally sponsored effort, and we'll need multiple PRs to implement the desired functionality. I beg for some feedback loop fluency here.

I'll start figuring out the code for the feature flags, but feedback on rewriting everything to Kotlin will also be extremely helpful.

Please let me know if I, or Expensify, can do anything to improve the feedback loop situation.

[mdvacca]

@cubuspl42, will take a look at it today, thank you!

[cubuspl42]

@mdvacca Thank you!

[mdvacca]

@cubuspl42

From the operational perspective, you're talking about A/B tests that Meta could run on the Facebook app and/or other Meta apps, do I get this right?

Correct, technically this would mean to add a new static variable in ReactFeatureFlags and then refactor the PR to be able to execute "old Code" or "new code" based on the value of the feature flag.

https://github.com/facebook/react-native/blob/97647d11841c9c97632617fe770439e3acb689ea/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java

We will take care of running the experiment at Meta and analyzing result of it.

I will share some detail of the code in a bit

[mdvacca]

Thanks for moving the new classes to internal and migrating them to Kotlin!

Would you provide a few lines of code that show how things would look after the refactor so I'm sure we're on the same page?

I did a further analysis and I think that using a builder pattern will require a much bigger refactor, I think we could leave it like this for now.

Although it will important to add a FeatureFlag as I mentioned in my comment above.

Also, let's document all new public classes, interfaces and method using KDoc

@cortinico, can you also take a quick look at this PR? in particular the Kotlin side

Thanks!

[mdvacca]

Let's say you create a feature flag called ReactFeatureFlags.enableSpannableCreationUnification, then here's one of the places where you have to gate your new code:

if (enableSpannableCreationUnification) {
   textFragmentList = new BridgeTextFragmentList(fragments);
   ...
} else {
  for (int i = 0, length = fragments.size(); i < length; i++) {
  ....
  }
}

I know this make this code a bit dirty, but it would be important to understand the impact of this PR.
We will cleanup the code after we confirm that this PR works as expected.

[cubuspl42]

Done

[cubuspl42]

Also, let's document all new public classes, interfaces and methods using KDoc

If I'm not mistaken, I added KDocs to all new classes. Are you sure about the methods? Should I add comment docs for all Java getters, Kotlin properties, the new addFoo helpers, etc.?

[TheRogue76]

It would be nice, we are also exploring doing a V7 for lottie to address this and the new install_module script on iOS (since it only supports RN 70 and above). But it would be nice to not have to worry about this too much 🙏🏻

[cubuspl42]

@Kudo

i would just patch lottie-react-native for replacing UNSET to -1

I'm not an authoritative source here, but I'm 90% sure this is now considered a public API:

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

Line 20 in 7bcdb23

public static final int UNSET = -1;

I'd use this on your place as a fix for the specific problem you encountered.

[Kudo]

@cubuspl42 sounds good! i'll update the patch accordingly. thanks for the great hint.

[cubuspl42]

For future reference, there's one other local definition of UNSET:

react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/TouchEvent.java

Line 33 in 714b502

public static final long UNSET = Long.MIN_VALUE;

If it ever needs extracting, we could name it ReactConstants.UNSET_LONG_MIN. However, it might never be necessary.

I don't think we have any cases of UNSET = -1L. If we need it, we could name it ReactConstants.UNSET_LONG.

I think we don't need more future predicting here.

[cubuspl42]

@NickGerleman Kind bump on the experiment info

What's the ETA for starting and finishing it?

[NickGerleman]

@NickGerleman Kind bump on the experiment info

What's the ETA for starting and finishing it?

Because of the nature of needing to incrementally roll out to production audience, and get feedback, it will be a while before we are able to remove the forking (potentially a month or more). But it should be possible to continue building on top of it in the meantime.

[cubuspl42]

@NickGerleman Thanks!

As I understand it, it's already live, and the finishing ETA is roughly 1-3 months. I don't mean to push it, I just want to establish a timeline.

[NickGerleman]

Hey @cubuspl42, while rolling this out to our users, we discovered it causes a performance regression, in the time to render content, for a couple of our scenarios. More concretely, a number of users in the millions would go from "responsive" to "unresponsive" on those surfaces.

I have deallocated the experiment due to performance impact. If we wanted to try shipping this again, I think we'd need to understand where this issue presents in real apps, and come up with a fix.

[cubuspl42]

Thank you for sharing these results! I must admit that they surprised me, but prove you had a good intuition to set up an experiment. Did the mentioned user group share a common configuration of React Native? Was it Fabric/bridgeless?

[NickGerleman]

The surfaces were Fabric + Bridgeless. One involved a scrollable grid of components with text. One was more static. Both in an app with very large usage.

A lot of real-world Android devices are low/middle-end, which means sometimes a very minor change on higher end device, can push a surprisingly large number of real-world devices from one responsiveness threshold to another.

[cubuspl42]

It was brought to my attention that someone from Meta (I'm not sure whether it was you personally) contacted Expensify, informing that Meta will no longer accept big PRs related to Text in the foreseeable future.

1. Would you like me to submit a follow-up that removes the "unified" path, or would you prefer to leave it there, so someone from Meta can investigate this topic in the future, when this will be more aligned with Meta's roadmap and priorities?

2. Other folks from Expensify are wondering whether adding test suites of some kind could change Meta's position on this matter. If you know that the answer is "definitely no," I could forward this information, and it would speed up the pivoting process on our side (possibly by months).

Thank you for your help so far; I'm sorry it ended this way.

[NickGerleman]

Hey there. I think @TheSavior chatted with folks at Expensify wrt Text changes.

Our own problem right now, is that certain classes of contributions are hard to land, from outside of Meta. In these cases, where we have encouraged contribution, we've often created a bad experience for change authors (hard/slow to merge, hard to debug regressions effecting product code), coupled with at times a spikey workload for engineers at Meta, who usually don't have time budgeted ahead of time for larger changes outside their current area of focus.

I don't know that we ever explicitly chatted about what to do with in-flight changes. If there is a scoped, logical wrap-up, to changes already in flight, I think we could take a look at those. But if we think this is more likely to become dead code, I do really appreciate the offer to help clean things up.

I really appreciate all the time you spent here, working to make RN better. I am sorry, we couldn't offer a better experience here.

[elicwhite]

I do think it is a reasonable question of whether there is something that could be contributed around testing to increase our confidence in merging changes to Text. @NickGerleman do you have any thoughts there?

[NickGerleman]

Meta has collectively spent some tens of thousand of hours on tooling and infra, but has usually focused on using them for Meta products, or the Meta monorepo. That puts us into a complicated state, where we do use this tooling, as a pragmatic way to ensure RN quality (for our apps, and OSS). But those tools don't come free to OSS.

Without commenting about what can be contributed vs not, there are a few classes of tooling that come to mind, that we use to ensure quality:

Fast and stable screenshot comparison testing

We get feedback on whether a change visually effects both our real-world surfaces, and fixtures we created. E.g. when I was working on the Samsung TextInput SEV a while back, I set up infra to visually compare a lot of interesting formatting span/nesting scenarios, for both Fabric and Paper (and they have since caught real regressions).

There have been some efforts to stand up OSS E2E infra before, but they have run into stability issues, and never got around to a lot of what is really useful (e.g. screenshot comparison). If we had reliable, simple infra, to make it easy for contributors to add image baselines, that would give us a lot more confidence for some changes.

Experimentation infra

We often test by observing differences in behavior/performance when a change is enabled vs not. This is inherently something that has to come from a collection of product code (e.g. when I was at another company, we did the same thing). That looks like, enabling a change for a large fleet of users, across surfaces, and having good enough metrics to understand if we see e.g. more rage shakes, crashes, perf changes, impression changes, etc.

Perf infra

I know things like the Java sampling profiler work pretty out of the box in OSS (and I think, that is probably the best place to start looking for diagnosing a perf issue for e.g. a change like this). But there is often bespoke infa Meta has to make this sort of thing easier and faster. E.g. If I see that a change makes a surface slower to start, I can go look at how much time is spent, via a sampling profiler, that ran during the duration, collecting information across real production users. Or, tooling for e.g. trace markers, which I'm not sure are wired to sinks in most OSS builds (I remember when I was at MSFT, we did wire them to Windows ETW).

[NickGerleman]

To clarify though, I don't think better tooling is a magic bullet to ensure smooth contributions, though for many cases, I think it does help build confidence. I think another component, is aligning changes which will require a lot of interaction from engineers at Meta, to work the engineer is chartered to. That helps make it a lot easier to prioritize the work, and for someone to represent it or deal with any fallout when shipping it. I can say personally, at any given moment, there will be something falling off my plate that someone has asked me to do, just by the nature of the scale factor of the relatively small number of folks working on RN, compared to the very large userbase. So, it's easier to keep things above that line, when we can very easily justify the work.

[cubuspl42]

These are extremely helpful thoughts; this is a communication breakthrough from my perspective.

I hope I'm not stepping out of the line here, but this has been bothering me:

Would it be possible for you to talk with your superiors and discuss Meta's priorities in the context of React Native within its Open Source umbrella project? Would be possible to establish a budget for this "communitization" effort of React Native? A few talented engineers from the Expensify project (and the supporting companies, like Software Mansion) would love and be able to help with this, but I'm not going to lie; these are very difficult technical tasks requiring expert knowledge, experience and time.

The budget established for implementing spans (eventually, inline code blocks) in Text wouldn't cover implementing the testing infrastructure prototype. Also, it may not directly fall into a category of efforts that Expensify could manage to sponsor in general. The gains from these efforts would benefit the whole community, though.

[fabOnReact]

The facebook/react-native Android Test Suite was disabled for components that rely on Yoga for layout (commit 709a441).
It could be solved using the mock YogaConfigFactory to run those tests with roboeletric.

I don't believe Yoga layout is essential to run tests for Text and TextInput.

• We could just mock the height/width of the EditText or TextView. No need to compute height/width with Yoga for the test case.
• The Text/TextInput spans do not use Yoga Layout. It is an Android API named Spans. We don't need to have Yoga to verify that they work correctly.

More info in PR #35949 and more useful code pointers in comment #35130 (comment).