From 67c852e82fe64ddd6555b0ead08bb50db9cace86 Mon Sep 17 00:00:00 2001 From: Samuel Susla Date: Fri, 24 Nov 2023 04:09:14 -0800 Subject: [PATCH] filter out yoga style props during folly::dynamic conversion (#41607) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/41607 X-link: https://github.com/facebook/hermes/pull/1198 changelog: [internal] Reviewed By: javache Differential Revision: D51471665 fbshipit-source-id: 0633422d356c2f2419ea87b2a2ad6b5ea64c99f6 --- .../ReactCommon/jsi/jsi/JSIDynamic.cpp | 11 +- .../ReactCommon/jsi/jsi/JSIDynamic.h | 3 +- .../components/view/YogaStylableProps.cpp | 127 +----------------- .../core/ConcreteComponentDescriptor.h | 7 + .../react/renderer/core/RawProps.cpp | 99 +++++++++++++- .../react/renderer/core/RawProps.h | 11 ++ .../renderer/core/tests/RawPropsTest.cpp | 36 +++++ 7 files changed, 169 insertions(+), 125 deletions(-) diff --git a/packages/react-native/ReactCommon/jsi/jsi/JSIDynamic.cpp b/packages/react-native/ReactCommon/jsi/jsi/JSIDynamic.cpp index 02f112c0e0ca09..5a2d38d9dd3915 100644 --- a/packages/react-native/ReactCommon/jsi/jsi/JSIDynamic.cpp +++ b/packages/react-native/ReactCommon/jsi/jsi/JSIDynamic.cpp @@ -150,7 +150,10 @@ void dynamicFromValueShallow( } // namespace -folly::dynamic dynamicFromValue(Runtime& runtime, const Value& valueInput) { +folly::dynamic dynamicFromValue( + Runtime& runtime, + const Value& valueInput, + std::function filterObjectKeys) { std::vector stack; folly::dynamic ret; @@ -182,13 +185,17 @@ folly::dynamic dynamicFromValue(Runtime& runtime, const Value& valueInput) { if (prop.isUndefined()) { continue; } + auto nameStr = name.utf8(runtime); + if (filterObjectKeys && filterObjectKeys(nameStr)) { + continue; + } // The JSC conversion uses JSON.stringify, which substitutes // null for a function, so we do the same here. Just dropping // the pair might also work, but would require more testing. if (prop.isObject() && prop.getObject(runtime).isFunction(runtime)) { prop = Value::null(); } - props.emplace_back(name.utf8(runtime), std::move(prop)); + props.emplace_back(std::move(nameStr), std::move(prop)); top.dyn->insert(props.back().first, nullptr); } for (const auto& prop : props) { diff --git a/packages/react-native/ReactCommon/jsi/jsi/JSIDynamic.h b/packages/react-native/ReactCommon/jsi/jsi/JSIDynamic.h index 110dd13999f437..a96cc281b0d65b 100644 --- a/packages/react-native/ReactCommon/jsi/jsi/JSIDynamic.h +++ b/packages/react-native/ReactCommon/jsi/jsi/JSIDynamic.h @@ -19,7 +19,8 @@ facebook::jsi::Value valueFromDynamic( folly::dynamic dynamicFromValue( facebook::jsi::Runtime& runtime, - const facebook::jsi::Value& value); + const facebook::jsi::Value& value, + std::function filterObjectKeys = nullptr); } // namespace jsi } // namespace facebook diff --git a/packages/react-native/ReactCommon/react/renderer/components/view/YogaStylableProps.cpp b/packages/react-native/ReactCommon/react/renderer/components/view/YogaStylableProps.cpp index f8a7f360008d27..39e32c9fa93283 100644 --- a/packages/react-native/ReactCommon/react/renderer/components/view/YogaStylableProps.cpp +++ b/packages/react-native/ReactCommon/react/renderer/components/view/YogaStylableProps.cpp @@ -19,134 +19,19 @@ namespace facebook::react { -namespace { -inline RawProps filterYogaProps(const RawProps& rawProps) { - const static std::unordered_set yogaStylePropNames = { - {"direction", - "flexDirection", - "justifyContent", - "alignContent", - "alignItems", - "alignSelf", - "position", - "flexWrap", - "display", - "flex", - "flexGrow", - "flexShrink", - "flexBasis", - "margin", - "padding", - "rowGap", - "columnGap", - "gap", - // TODO: T163711275 also filter out width/height when SVG no longer read - // them from RawProps - "minWidth", - "maxWidth", - "minHeight", - "maxHeight", - "aspectRatio", - - // edges - "left", - "right", - "top", - "bottom", - "start", - "end", - - // variants of inset - "inset", - "insetStart", - "insetEnd", - "insetInline", - "insetInlineStart", - "insetInlineEnd", - "insetBlock", - "insetBlockEnd", - "insetBlockStart", - "insetVertical", - "insetHorizontal", - "insetTop", - "insetBottom", - "insetLeft", - "insetRight", - - // variants of margin - "marginStart", - "marginEnd", - "marginInline", - "marginInlineStart", - "marginInlineEnd", - "marginBlock", - "marginBlockStart", - "marginBlockEnd", - "marginVertical", - "marginHorizontal", - "marginTop", - "marginBottom", - "marginLeft", - "marginRight", - - // variants of padding - "paddingStart", - "paddingEnd", - "paddingInline", - "paddingInlineStart", - "paddingInlineEnd", - "paddingBlock", - "paddingBlockStart", - "paddingBlockEnd", - "paddingVertical", - "paddingHorizontal", - "paddingTop", - "paddingBottom", - "paddingLeft", - "paddingRight"}}; - - auto filteredRawProps = (folly::dynamic)rawProps; - - auto it = filteredRawProps.items().begin(); - while (it != filteredRawProps.items().end()) { - auto key = it->first.asString(); - if (yogaStylePropNames.find(key) != yogaStylePropNames.end()) { - it = filteredRawProps.erase(it); - } else { - ++it; - } - } - - return RawProps(std::move(filteredRawProps)); -} -} // namespace - YogaStylableProps::YogaStylableProps( const PropsParserContext& context, const YogaStylableProps& sourceProps, const RawProps& rawProps) : Props() { - if (CoreFeatures::excludeYogaFromRawProps) { - const auto filteredRawProps = filterYogaProps(rawProps); - initialize(context, sourceProps, filteredRawProps); - - yogaStyle = CoreFeatures::enablePropIteratorSetter - ? sourceProps.yogaStyle - : convertRawProp(context, filteredRawProps, sourceProps.yogaStyle); - - if (!CoreFeatures::enablePropIteratorSetter) { - convertRawPropAliases(context, sourceProps, filteredRawProps); - } - } else { - initialize(context, sourceProps, rawProps); + initialize(context, sourceProps, rawProps); - yogaStyle = CoreFeatures::enablePropIteratorSetter - ? sourceProps.yogaStyle - : convertRawProp(context, rawProps, sourceProps.yogaStyle); + yogaStyle = CoreFeatures::enablePropIteratorSetter + ? sourceProps.yogaStyle + : convertRawProp(context, rawProps, sourceProps.yogaStyle); - if (!CoreFeatures::enablePropIteratorSetter) { - convertRawPropAliases(context, sourceProps, rawProps); - } + if (!CoreFeatures::enablePropIteratorSetter) { + convertRawPropAliases(context, sourceProps, rawProps); } }; diff --git a/packages/react-native/ReactCommon/react/renderer/core/ConcreteComponentDescriptor.h b/packages/react-native/ReactCommon/react/renderer/core/ConcreteComponentDescriptor.h index a527d267eb41dd..ad8e60fb541da8 100644 --- a/packages/react-native/ReactCommon/react/renderer/core/ConcreteComponentDescriptor.h +++ b/packages/react-native/ReactCommon/react/renderer/core/ConcreteComponentDescriptor.h @@ -105,6 +105,13 @@ class ConcreteComponentDescriptor : public ComponentDescriptor { return ShadowNodeT::defaultSharedProps(); } + if (CoreFeatures::excludeYogaFromRawProps) { + if (ShadowNodeT::IdentifierTrait() == + ShadowNodeTraits::Trait::YogaLayoutableKind) { + rawProps.filterYogaStylePropsInDynamicConversion(); + } + } + rawProps.parse(rawPropsParser_); // Call old-style constructor diff --git a/packages/react-native/ReactCommon/react/renderer/core/RawProps.cpp b/packages/react-native/ReactCommon/react/renderer/core/RawProps.cpp index 046e8c521ae33a..3e630dcd0d042a 100644 --- a/packages/react-native/ReactCommon/react/renderer/core/RawProps.cpp +++ b/packages/react-native/ReactCommon/react/renderer/core/RawProps.cpp @@ -13,6 +13,96 @@ namespace facebook::react { +namespace { +inline bool isYogaStyleProp(const std::string& prop) { + const static std::unordered_set yogaStylePropNames = { + {"direction", + "flexDirection", + "justifyContent", + "alignContent", + "alignItems", + "alignSelf", + "position", + "flexWrap", + "display", + "flex", + "flexGrow", + "flexShrink", + "flexBasis", + "margin", + "padding", + "rowGap", + "columnGap", + "gap", + // TODO: T163711275 also filter out width/height when SVG no longer read + // them from RawProps + "minWidth", + "maxWidth", + "minHeight", + "maxHeight", + "aspectRatio", + + // edges + "left", + "right", + "top", + "bottom", + "start", + "end", + + // variants of inset + "inset", + "insetStart", + "insetEnd", + "insetInline", + "insetInlineStart", + "insetInlineEnd", + "insetBlock", + "insetBlockEnd", + "insetBlockStart", + "insetVertical", + "insetHorizontal", + "insetTop", + "insetBottom", + "insetLeft", + "insetRight", + + // variants of margin + "marginStart", + "marginEnd", + "marginInline", + "marginInlineStart", + "marginInlineEnd", + "marginBlock", + "marginBlockStart", + "marginBlockEnd", + "marginVertical", + "marginHorizontal", + "marginTop", + "marginBottom", + "marginLeft", + "marginRight", + + // variants of padding + "paddingStart", + "paddingEnd", + "paddingInline", + "paddingInlineStart", + "paddingInlineEnd", + "paddingBlock", + "paddingBlockStart", + "paddingBlockEnd", + "paddingVertical", + "paddingHorizontal", + "paddingTop", + "paddingBottom", + "paddingLeft", + "paddingRight"}}; + + return yogaStylePropNames.find(prop) != yogaStylePropNames.end(); +} +} // namespace + RawProps::RawProps() { mode_ = Mode::Empty; } @@ -55,6 +145,7 @@ RawProps::RawProps(const RawProps& other) noexcept { } else if (mode_ == Mode::Dynamic) { dynamic_ = other.dynamic_; } + ignoreYogaStyleProps_ = other.ignoreYogaStyleProps_; } RawProps& RawProps::operator=(const RawProps& other) noexcept { @@ -65,6 +156,7 @@ RawProps& RawProps::operator=(const RawProps& other) noexcept { } else if (mode_ == Mode::Dynamic) { dynamic_ = other.dynamic_; } + ignoreYogaStyleProps_ = other.ignoreYogaStyleProps_; return *this; } @@ -84,12 +176,17 @@ RawProps::operator folly::dynamic() const noexcept { case Mode::Empty: return folly::dynamic::object(); case Mode::JSI: - return jsi::dynamicFromValue(*runtime_, value_); + return jsi::dynamicFromValue( + *runtime_, value_, ignoreYogaStyleProps_ ? isYogaStyleProp : nullptr); case Mode::Dynamic: return dynamic_; } } +void RawProps::filterYogaStylePropsInDynamicConversion() noexcept { + ignoreYogaStyleProps_ = true; +} + /* * Returns `true` if the object is empty. * Empty `RawProps` does not have any stored data. diff --git a/packages/react-native/ReactCommon/react/renderer/core/RawProps.h b/packages/react-native/ReactCommon/react/renderer/core/RawProps.h index c08cf928a508bd..7edaa8a7ed30f0 100644 --- a/packages/react-native/ReactCommon/react/renderer/core/RawProps.h +++ b/packages/react-native/ReactCommon/react/renderer/core/RawProps.h @@ -73,6 +73,15 @@ class RawProps final { */ explicit operator folly::dynamic() const noexcept; + /* + * Once called, Yoga style props will be filtered out during conversion to + * folly::dynamic. folly::dynamic conversion is only used on Android and props + * specific to Yoga do not need to be send over JNI to Android. + * This is a performance optimisation to minimise traffic between C++ and + * Java. + */ + void filterYogaStylePropsInDynamicConversion() noexcept; + /* * Returns `true` if the object is empty. * Empty `RawProps` does not have any stored data. @@ -124,6 +133,8 @@ class RawProps final { */ mutable std::vector keyIndexToValueIndex_; mutable std::vector values_; + + bool ignoreYogaStyleProps_{false}; }; } // namespace facebook::react diff --git a/packages/react-native/ReactCommon/react/renderer/core/tests/RawPropsTest.cpp b/packages/react-native/ReactCommon/react/renderer/core/tests/RawPropsTest.cpp index 9b57fe14586d62..ea9e657a428791 100644 --- a/packages/react-native/ReactCommon/react/renderer/core/tests/RawPropsTest.cpp +++ b/packages/react-native/ReactCommon/react/renderer/core/tests/RawPropsTest.cpp @@ -467,3 +467,39 @@ TEST(RawPropsTest, copyJSIRawProps) { EXPECT_NEAR( copyProps->derivedFloatValue, originalProps->derivedFloatValue, 0.00001); } + +TEST(RawPropsTest, filterYogaRawProps) { + auto runtime = facebook::hermes::makeHermesRuntime(); + + ContextContainer contextContainer{}; + PropsParserContext parserContext{-1, contextContainer}; + + auto object = jsi::Object(*runtime); + object.setProperty(*runtime, "floatValue", 10.0); + object.setProperty(*runtime, "flex", 1); + + auto rawProps = RawProps(*runtime, jsi::Value(*runtime, object)); + + EXPECT_FALSE(rawProps.isEmpty()); + + auto dynamicProps = (folly::dynamic)rawProps; + + EXPECT_EQ(dynamicProps["floatValue"], 10.0); + EXPECT_EQ(dynamicProps["flex"], 1); + + rawProps.filterYogaStylePropsInDynamicConversion(); + + dynamicProps = (folly::dynamic)rawProps; + + EXPECT_EQ(dynamicProps["floatValue"], 10.0); + EXPECT_EQ(dynamicProps["flex"], nullptr); + + // The fact that filterYogaStylePropsInDynamicConversion should + // must apply to a copy as well. + auto copy = RawProps(rawProps); + + auto dynamicPropsFromCopy = (folly::dynamic)copy; + + EXPECT_EQ(dynamicPropsFromCopy["floatValue"], 10.0); + EXPECT_EQ(dynamicPropsFromCopy["flex"], nullptr); +}