feat: flex gap bindings

[intergalacticspacehighway]

Summary

This PR adds React native binding for facebook/yoga#1116

Changelog

[General] [Added] - Flex gap yoga bindings

Test Plan

Run rn tester app and go to view example. You'll find a flex gap example. Example location - packages/rn-tester/js/examples/View/ViewExample.js

Tested on

• iOS Fabric
• iOS non-fabric
• Android Fabric
• Android non-fabric

To test on non-fabric Android, I just switched these booleans. Let me know if there's anything else I might have missed.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,777,989	+24,041
android	hermes	armeabi-v7a	7,180,388	+22,173
android	hermes	x86	8,091,972	+26,224
android	hermes	x86_64	8,062,894	+26,497
android	jsc	arm64-v8a	9,637,441	+24,588
android	jsc	armeabi-v7a	8,401,709	+22,709
android	jsc	x86	9,586,633	+26,755
android	jsc	x86_64	10,179,794	+27,034

Base commit: 08dafd8
Branch: main

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 45db65b
Branch: main

[NickGerleman]

Thank you for putting this together. It looks solid to me on first glance, but I want to take a closer look early next week.

On the last PR I mentioned a concern over how this change intersects with the way React Native passes style properties to native components. They are flattened, so Shadow Nodes are given "gap" instead of "style.gap". So these properties are in a top level namespace shared with any native component a project or library might have added.

I really want to see this change merged, and have been thinking about how we might be able to handle the issue. I have two trains of thought on how we might be able to do that.

1. Namespace the style props: The right architecture move is to keep these namespaced, but it's tricky to get right. It intersects with the React renderer, every native platform, and needs to be staged between native and JS. We may not want to namespace the existing props, both for staging, and to allow existing view managers spying on them to still work. We could maybe add namespaced style props, but keep the existing ones at the top level for compatibility. Still need to wrap my head around some of the internals.
2. Investigate real-world impact: Check to see if these would conflict with examples we can find in the wild. We can't see every codebase, but we can probably make some guesses if we search through large codebases (e.g. for usage of "gap", "rowGap", "columnGap" in an @ReactProp annotations). Once we have these strings for the various platforms enumerated, we can search GitHub, Meta's codebase, to see if we are likely to cause breakage in practice.

[jacobp100]

@NickGerleman You're completely correct that this will impact poorly written existing code. I had a look at GH search, FluentUI, near-mobile-wallet, and topotal/js-sdk were responsible for most results - and their implementations will work.

I did find three places (1, 2 & 3) where these properties are set on RN components directly, so do hit the case you mentioned. The last two actually won't have an impact due to scrollviews having a single container child - but that's just by chance.

I worry that if we look into adding namespacing, this PR will never get merged.

Maybe we could add some checks on View to remove gap-related properties from the props, and do a console.warn to let them know they need to fix their code. In a later version, we can remove these checks and warning. This approach does have edge-cases - the people setting these props on scrollviews won't get the warning, but as mentioned before, it won't actually impact them. Hopefully this will be a headlining feature in the change-log, so we can highlight this issue there.

Let me know what you think of that solution!

[NickGerleman]

For the breaks you pointed out are where gap was being leaked to a View, so a property that used to no-op no longer does. Your idea to add a check in View to ward these off makes sense to me, if we want to take the caution.

Just to clarify, for the first paragraph, were these hits the case where a native component allows gap/rowGap/columnGap as a prop? The concern there is that components using APIs like SimpleViewManager may previously have allowed gap as a valid custom property, but will conflict after this change, and I'm not sure how that is surfaced (i.e. at build time or runtime). I did not find any instances of gap/rowGap/columnGap in at least ReactProp or RCT_EXPORT_VIEW_PROPERTY internally.

[intergalacticspacehighway]

that components using APIs like SimpleViewManager

@NickGerleman makes sense. Should it be fine as long as these libraries are able to migrate maybe by not using SimpleViewManager and creating a custom one? (renaming that prop would be much easier though)

One more suggestion could be to write a script that checks if a native view is using ReactProp or RCT_EXPORT_VIEW_PROPERTY with gap prop and warn to migrate?

warning where gap was being leaked to a View

We can do this, although libraries like styled-components pass it in style prop so it won't cover if it's leaked from there. But I feel this case is more like a refactor on the user end. Something that was not working now works (but I could be wrong! 😅)

[jacobp100]

@NickGerleman No - they were JS-only components. They'd just do React.Children.map to add margins of half the gap

For the native components where they wish to take a top-level gap prop - I think that's still fine. Although I think it will also be possible to set that by the style prop too

@intergalacticspacehighway just to clarify - I was proposing adding the checks in View.js

[NickGerleman]

For the native components where they wish to take a top-level gap prop - I think that's still fine. Although I think it will also be possible to set that by the style prop too

I am going to try to test the behavior of having conflicting props in the inheritance chain today, and if it results in either build errors, runtime errors, or if there is a given precedence. Build errors are fine, and I think it would also be effectively fine (but smelly) if props are set for both the native component and yoga node, since gap won't affect the layout of some leaf-node native component. But the bad case would be if someone had some Carousel native component with an unrelated gap, and all of a sudden it either no longer receives the prop (inner-most/first handler takes precedence), or there is a runtime error. Though I suspect even if we run into one of these bad cases, we still might be able to punt for now depending on the code out there.

After this change I do really want to take a closer look at name-spacing. Ideally we wouldn't have to reason about all of these edge cases.

[jacobp100]

@NickGerleman I had a look on GitHub and couldn't find any instances of a native component having a gap property. I only found the instances before where it's getting set on a <View> or <ScrollView> (as mentioned before)

[NickGerleman]

@NickGerleman I had a look on GitHub and couldn't find any instances of a native component having a gap property. I only found the instances before where it's getting set on a <View> or <ScrollView> (as mentioned before)

Ah sorry, I think I misinterpreted your earlier message. I think we're probably fine to go on that front then. For the scenario where folks are passing a bunch of extra props to <View>, I think we probably don't strictly need the warning, since folks really shouldn't be passing a bunch of extra props that may eventually do something one day.

I looked through the PR and everything looks good. So, we should be ready to import after:

1. Fixing up the Flow/ESLint Errors (See test_js and analyze_code CircleCI jobs)
2. Update the comment sectioning in StyleSheetTypes as-per feat: flex gap bindings #34974 (comment)

[jacobp100]

@NickGerleman I was proposing we add something like this. There were a few examples of people over-setting properties. But to be honest, it might be easier to just ping them and let them know - rather than go through all the rigmarole of adding these checks, then deleting them later

[facebook-github-bot]

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

[NickGerleman]

It looks like this change is causing a test to fail internally, but is otherwise approved. Root causing that, then we should be able to merge.

[react-native-bot]

This pull request was successfully merged by @intergalacticspacehighway in 9f3a3e1.

^{When will my fix make it into a release? | Upcoming Releases}

[matthew-dean]

Soooo close. Please release 0.71.0 with gap support!