Move all ReactSpan subclasses to a separate folder

[jakub-trzebiatowski]

Summary:

Move all ReactSpan subclasses to a separate folder.

This is a minor improvement in the context of my multi-PR work on react-native-community/discussions-and-proposals#695.

I'm adding a new span class later, which was the direct motivation for this change.

Changelog:

[INTERNAL] [CHANGE] - Move all ReactSpan subclasses to a separate folder

Test Plan:

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	16,972,236	+8,459
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	20,355,997	+8,447
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 5f75e9b
Branch: main

[jakub-trzebiatowski]

@mdvacca Could you review this?

[jakub-trzebiatowski]

@mdvacca There were already quite a few Span subclasses in the package, and I'm about to add at least one more. I thought it would be clearer if we put it inside a sub-package. It's not a big deal actually 🙂

[mdvacca]

Unfortunately this is a breakage of compatibility, could you analyze the usage of these classes by Open Source third party libraries in github search?

cc @cortinico

[jakub-trzebiatowski]

@mdvacca It's a great point! I agree that we should be careful here.

I actually gave a thought in the context of this PR, and I concluded that there's no reasonable reason to use our Span subclasses from the outside. They feel to be total implementation details.

But I'll search for usages anyway! Thanks for the tip.

[cortinico]

Unfortunately this is a breakage of compatibility, could you analyze the usage of these classes by Open Source third party libraries in github search?
cc @cortinico

@cubuspl42 here some usages:
https://github.com/search?type=code&q=import+%2Fcom.facebook.react.views.text.*Span%2F+NOT+is%3Afork+language%3AJava&p=5

Most of the usages are copies of our classes (easily spottable if the filepath is com/facebook/react).
Other usages are related to old libraries, so the impact is limited IMHO.

@mdvacca could we move this to .internal maybe then?

[jakub-trzebiatowski]

Moving anything to .internal hardly works, because the internal Meta build system doesn't allow cycles between Java packages. Yeah, I mean actual Java packages.

I had to revert all moving to .internal in #39630.

It would be great to figure this out.

[jakub-trzebiatowski]

Using Kotlin internal visibility modifier also doesn't work.

[cortinico]

Moving anything to .internal hardly works, because the internal Meta build system doesn't allow cycles between Java packages. Yeah, I mean actual Java packages.

mmm how is com.facebook.react.views.text.span different from com.facebook.react.internal.text.span in terms of cycles?

[cortinico]

Using Kotlin internal visibility modifier also doesn't work.

Using the internal modifier is sadly not an option for us because we use BUCK internally which treat each Java package as a separate library. So things will be internal to the package (i.e. same visibility as package private).

[jakub-trzebiatowski]

Moving anything to .internal hardly works, because the internal Meta build system doesn't allow cycles between Java packages. Yeah, I mean actual Java packages.

mmm how is com.facebook.react.views.text.span different from com.facebook.react.internal.text.span in terms of cycles?

I assumed we'd create a cycle between com.facebook.react.internal.views.text and com.facebook.react.views.text. It happened immediately in the previous PR. But maybe it indeed doesn't happen here. I can check.

[cortinico]

I assumed we'd create a cycle between com.facebook.react.internal.views.text and com.facebook.react.views.text. It happened immediately in the previous PR. But maybe it indeed doesn't happen here. I can check.

Ah yeah that can be the case. Still if so, we'll have the cycle anyway between com.facebook.react.views.text and com.facebook.react.views.text.span.

As a last resort we could also use com.facebook.react.views.text.internal.span. It's not preferred as we tend to have everything internal in the top level .internal package, but when not possible that's also an option

[jakub-trzebiatowski]

I'll try it.

Have you considered adding some annotation for React-internal APIs? Sure, it's not as bullet-proof as the .internal infix in the fully qualified name (someone might not notice the annotation), but at least it makes it clear from our side what is and what isn't considered stable and supported for the community dependencies with minimal changes to the RN actual code.

Also, it makes it possible to annotate methods inside an otherwise public class.

[cortinico]

Have you considered adding some annotation for React-internal APIs?

Yes we do already have some:

react-native/packages/react-native/ReactAndroid/gradle.properties

Lines 25 to 26 in b8778ab

	binaryCompatibilityValidator.nonPublicMarkers=com.facebook.react.common.annotations.VisibleForTesting,\
	com.facebook.react.common.annotations.UnstableReactNativeAPI

We've been discussing with @mdvacca to actually add an @Internal or @ReactNativeInternal annotation as well exactly for the reasons you mentioned

[jakub-trzebiatowski]

Yes we do already have some

I don't think that they match my use-case, though. @VisibleForTesting is clearly specific for stuff visible only for testing, while "unstable" sounds like "public, but experimental and might change".

We've been discussing with @mdvacca to actually add an @internal or @ReactNativeInternal

Interesting! A remaining question is whether this would be a replacement or a supplement to the .internal convention.

[cortinico]

Interesting! A remaining question is whether this would be a replacement or a supplement to the .internal convention.

It's going to be a supplement as we intend to have .internal as primary way to indicate non public APIs

[jakub-trzebiatowski]

@cortinico Could you take a look now?

If the checks succeed, does it mean Buck accepted this code?

[cortinico]

If the checks succeed, does it mean Buck accepted this code?

Nope till we import it. Let me look into it

[facebook-github-bot]

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

[cortinico]

This is problematic as ReactTextInlineImageShadowNode (which is public) has this method:

public abstract class ReactTextInlineImageShadowNode extends LayoutShadowNode {
  /**
   * Build a {@link TextInlineImageSpan} from this node. This will be added to the TextView in place
   * of this node.
   */
  public abstract TextInlineImageSpan buildInlineImageSpan();
}

so we'll be returning an .internal object from a public class.

We probably should move also ReactTextInlineImageShadowNode to .internal

[jakub-trzebiatowski]

Done

[cortinico]

@cubuspl42 can we make sure the CI is green?

[facebook-github-bot]

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

[cortinico]

I'll drive this forward as there are various failures internally which needs to be handled.

[facebook-github-bot]

@cortinico merged this pull request in d4c3311.