Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: flex gap bindings #34974

Conversation

intergalacticspacehighway
Copy link
Contributor

@intergalacticspacehighway intergalacticspacehighway commented Oct 13, 2022

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.

Screenshot 2022-10-14 at 3 30 48 AM

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 13, 2022
@react-native-bot react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Oct 13, 2022
@analysis-bot
Copy link

analysis-bot commented Oct 13, 2022

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
Copy link

analysis-bot commented Oct 13, 2022

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

Base commit: 45db65b
Branch: main

@NickGerleman
Copy link
Contributor

NickGerleman commented Oct 14, 2022

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
Copy link
Contributor

jacobp100 commented Oct 17, 2022

@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
Copy link
Contributor

NickGerleman commented Oct 19, 2022

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
Copy link
Contributor Author

intergalacticspacehighway commented Oct 19, 2022

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
Copy link
Contributor

jacobp100 commented Oct 19, 2022

@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
Copy link
Contributor

NickGerleman commented Oct 19, 2022

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
Copy link
Contributor

jacobp100 commented Oct 19, 2022

@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
Copy link
Contributor

@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
Copy link
Contributor

jacobp100 commented Oct 19, 2022

@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
Copy link
Contributor

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

@NickGerleman
Copy link
Contributor

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
Copy link
Collaborator

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

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

@matthew-dean
Copy link

Soooo close. Please release 0.71.0 with gap support!

facebook-github-bot pushed a commit that referenced this pull request Dec 5, 2022
Summary:
Missed while reviewing #34974 that flex gap props in Flow typings were added to ShadowStyle instead of LayoutStyle (directly above). The same issue is not present in the TS typings.

Changelog:
[General][Fixed] - Move flex gap props to the correct type

Reviewed By: yungsters

Differential Revision: D41736652

fbshipit-source-id: de0db1676464fbd962b2c7e7e9ef62b795332f1b
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
This PR adds React native binding for facebook/yoga#1116

## Changelog

[General] [Added] - Flex gap yoga bindings

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

Pull Request resolved: facebook#34974

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
- [x] iOS Fabric
- [x] iOS non-fabric
- [x] Android Fabric
- [x] 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.

<img width="674" alt="Screenshot 2022-10-14 at 3 30 48 AM" src="https://user-images.githubusercontent.com/23293248/195718971-7aee4e7e-dbf0-4452-9d47-7925919c61dc.png">

Reviewed By: mdvacca

Differential Revision: D40527474

Pulled By: NickGerleman

fbshipit-source-id: 81c2c97c76f58fad3bb40fb378aaf8b6ebd30d63
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
Missed while reviewing facebook#34974 that flex gap props in Flow typings were added to ShadowStyle instead of LayoutStyle (directly above). The same issue is not present in the TS typings.

Changelog:
[General][Fixed] - Move flex gap props to the correct type

Reviewed By: yungsters

Differential Revision: D41736652

fbshipit-source-id: de0db1676464fbd962b2c7e7e9ef62b795332f1b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants