From f303266d697c7c830e845352d304fc621a355e73 Mon Sep 17 00:00:00 2001 From: Samuel Susla Date: Mon, 22 Feb 2021 05:50:39 -0800 Subject: [PATCH] Correctly handle null values in dynamic props in prop forwarding Summary: Changelog: [internal] shergin found that folly's merge_patch implementation doesn't propagate `null` correctly (details in D26435620 (https://github.com/facebook/react-native/commit/1e9f63fe277c42d812ef007ced7eff1688602b62)). This is a requirement and needs to be adjusted in props forwarding on Android. As far as we know this isn't causing any bugs but it is an error that should be fixed. Reviewed By: shergin Differential Revision: D26545821 fbshipit-source-id: 9edd24aecfcde17f5d9c1197f65db0e0f3f9e364 --- .../LegacyViewManagerInteropViewProps.cpp | 26 +------- .../renderer/core/DynamicPropsUtilities.cpp | 35 ++++++++++ .../renderer/core/DynamicPropsUtilities.h | 20 ++++++ .../react/renderer/core/ShadowNode.cpp | 6 +- .../core/tests/DynamicPropsUtilitiesTest.cpp | 66 +++++++++++++++++++ .../core/tests/FollyDynamicMergePatchTest.cpp | 56 ---------------- 6 files changed, 127 insertions(+), 82 deletions(-) create mode 100644 ReactCommon/react/renderer/core/DynamicPropsUtilities.cpp create mode 100644 ReactCommon/react/renderer/core/DynamicPropsUtilities.h create mode 100644 ReactCommon/react/renderer/core/tests/DynamicPropsUtilitiesTest.cpp delete mode 100644 ReactCommon/react/renderer/core/tests/FollyDynamicMergePatchTest.cpp diff --git a/ReactCommon/react/renderer/components/legacyviewmanagerinterop/LegacyViewManagerInteropViewProps.cpp b/ReactCommon/react/renderer/components/legacyviewmanagerinterop/LegacyViewManagerInteropViewProps.cpp index 1bf993a655c..db6236e0743 100644 --- a/ReactCommon/react/renderer/components/legacyviewmanagerinterop/LegacyViewManagerInteropViewProps.cpp +++ b/ReactCommon/react/renderer/components/legacyviewmanagerinterop/LegacyViewManagerInteropViewProps.cpp @@ -6,38 +6,18 @@ */ #include "LegacyViewManagerInteropViewProps.h" +#include namespace facebook { namespace react { -static folly::dynamic mergeRawProps( - folly::dynamic const &source, - folly::dynamic const &patch) { - auto result = source; - - if (!result.isObject()) { - result = folly::dynamic::object(); - } - - if (!patch.isObject()) { - return result; - } - - // Note, here we have to preserve sub-prop objects with `null` value as - // an indication for the legacy mounting layer that it needs to clean them up. - for (auto const &pair : patch.items()) { - result[pair.first] = pair.second; - } - - return result; -} - LegacyViewManagerInteropViewProps::LegacyViewManagerInteropViewProps( const LegacyViewManagerInteropViewProps &sourceProps, const RawProps &rawProps) : ViewProps(sourceProps, rawProps), otherProps( - mergeRawProps(sourceProps.otherProps, (folly::dynamic)rawProps)) {} + mergeDynamicProps(sourceProps.otherProps, (folly::dynamic)rawProps)) { +} } // namespace react } // namespace facebook diff --git a/ReactCommon/react/renderer/core/DynamicPropsUtilities.cpp b/ReactCommon/react/renderer/core/DynamicPropsUtilities.cpp new file mode 100644 index 00000000000..428ae4f3b83 --- /dev/null +++ b/ReactCommon/react/renderer/core/DynamicPropsUtilities.cpp @@ -0,0 +1,35 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#include "DynamicPropsUtilities.h" + +namespace facebook { +namespace react { +folly::dynamic mergeDynamicProps( + folly::dynamic const &source, + folly::dynamic const &patch) { + auto result = source; + + if (!result.isObject()) { + result = folly::dynamic::object(); + } + + if (!patch.isObject()) { + return result; + } + + // Note, here we have to preserve sub-prop objects with `null` value as + // an indication for the legacy mounting layer that it needs to clean them up. + for (auto const &pair : patch.items()) { + result[pair.first] = pair.second; + } + + return result; +} + +} // namespace react +} // namespace facebook diff --git a/ReactCommon/react/renderer/core/DynamicPropsUtilities.h b/ReactCommon/react/renderer/core/DynamicPropsUtilities.h new file mode 100644 index 00000000000..9e7283cb8fc --- /dev/null +++ b/ReactCommon/react/renderer/core/DynamicPropsUtilities.h @@ -0,0 +1,20 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#pragma once + +#include + +namespace facebook { +namespace react { + +folly::dynamic mergeDynamicProps( + folly::dynamic const &source, + folly::dynamic const &patch); + +} // namespace react +} // namespace facebook diff --git a/ReactCommon/react/renderer/core/ShadowNode.cpp b/ReactCommon/react/renderer/core/ShadowNode.cpp index 60f63ff3e72..4a0d8d3fb07 100644 --- a/ReactCommon/react/renderer/core/ShadowNode.cpp +++ b/ReactCommon/react/renderer/core/ShadowNode.cpp @@ -7,6 +7,7 @@ #include "ShadowNode.h" #include "Constants.h" +#include "DynamicPropsUtilities.h" #include "ShadowNodeFragment.h" #include @@ -42,10 +43,9 @@ SharedProps ShadowNode::propsForClonedShadowNode( bool hasBeenMounted = sourceShadowNode.hasBeenMounted_; bool sourceNodeHasRawProps = !sourceShadowNode.getProps()->rawProps.empty(); if (!hasBeenMounted && sourceNodeHasRawProps && props) { - auto copiedProps = sourceShadowNode.getProps()->rawProps; - copiedProps.merge_patch(props->rawProps); auto &castedProps = const_cast(*props); - castedProps.rawProps = copiedProps; + castedProps.rawProps = mergeDynamicProps( + sourceShadowNode.getProps()->rawProps, props->rawProps); return props; } } diff --git a/ReactCommon/react/renderer/core/tests/DynamicPropsUtilitiesTest.cpp b/ReactCommon/react/renderer/core/tests/DynamicPropsUtilitiesTest.cpp new file mode 100644 index 00000000000..b6666ad0842 --- /dev/null +++ b/ReactCommon/react/renderer/core/tests/DynamicPropsUtilitiesTest.cpp @@ -0,0 +1,66 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#include +#include + +using namespace folly; +using namespace facebook::react; + +/* + Tests that verify expected behaviour from `folly::dynamic::merge_patch`. + `merge_patch` is used for props forwarding on Android to enable Background + Executor and will be removed once JNI layer is reimplmeneted. + */ +TEST(DynamicPropsUtilitiesTest, handleNestedObjects) { + dynamic map1 = dynamic::object; + map1["style"] = dynamic::object("backgroundColor", "red"); + + dynamic map2 = dynamic::object; + map2["style"] = dynamic::object("backgroundColor", "blue")("color", "black"); + map2["height"] = 100; + + auto result = mergeDynamicProps(map1, map2); + + EXPECT_TRUE(result["style"].isObject()); + EXPECT_TRUE(result["style"]["backgroundColor"].isString()); + EXPECT_TRUE(result["style"]["color"].isString()); + EXPECT_TRUE(result["height"].isInt()); + + EXPECT_EQ(result["style"]["backgroundColor"].asString(), "blue"); + EXPECT_EQ(result["style"]["color"], "black"); + EXPECT_EQ(result["height"], 100); +} + +TEST(DynamicPropsUtilitiesTest, handleEmptyObject) { + dynamic map1 = dynamic::object; + + dynamic map2 = dynamic::object; + map2["height"] = 100; + + auto result = mergeDynamicProps(map1, map2); + + EXPECT_TRUE(result["height"].isInt()); + EXPECT_EQ(result["height"], 100); + + result = mergeDynamicProps(map1, map2); + + EXPECT_TRUE(result["height"].isInt()); + EXPECT_EQ(result["height"], 100); +} + +TEST(DynamicPropsUtilitiesTest, handleNull) { + dynamic map1 = dynamic::object; + map1["height"] = 100; + + dynamic map2 = dynamic::object; + map2["height"] = nullptr; + + auto result = mergeDynamicProps(map1, map2); + + EXPECT_TRUE(result["height"].isNull()); +} diff --git a/ReactCommon/react/renderer/core/tests/FollyDynamicMergePatchTest.cpp b/ReactCommon/react/renderer/core/tests/FollyDynamicMergePatchTest.cpp deleted file mode 100644 index 0362d6e0174..00000000000 --- a/ReactCommon/react/renderer/core/tests/FollyDynamicMergePatchTest.cpp +++ /dev/null @@ -1,56 +0,0 @@ -/* - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -#include - -#include -#include -#include - -using namespace folly; - -/* - Tests that verify expected behaviour from `folly::dynamic::merge_patch`. - `merge_patch` is used for props forwarding on Android to enable Background - Executor and will be removed once JNI layer is reimplmeneted. - */ -TEST(FollyDynamicMergePatchTest, handleNestedObjects) { - dynamic map1 = dynamic::object; - map1["style"] = dynamic::object("backgroundColor", "red"); - - dynamic map2 = dynamic::object; - map2["style"] = dynamic::object("backgroundColor", "blue")("color", "black"); - map2["height"] = 100; - - map2.merge_patch(map1); - - EXPECT_TRUE(map2["style"].isObject()); - EXPECT_TRUE(map2["style"]["backgroundColor"].isString()); - EXPECT_TRUE(map2["style"]["color"].isString()); - EXPECT_TRUE(map2["height"].isInt()); - - EXPECT_EQ(map2["style"]["backgroundColor"], "red"); - EXPECT_EQ(map2["style"]["color"], "black"); - EXPECT_EQ(map2["height"], 100); -} - -TEST(FollyDynamicMergePatchTest, handleEmptyObject) { - dynamic map1 = dynamic::object; - - dynamic map2 = dynamic::object; - map2["height"] = 100; - - map2.merge_patch(map1); - - EXPECT_TRUE(map2["height"].isInt()); - EXPECT_EQ(map2["height"], 100); - - map1.merge_patch(map2); - - EXPECT_TRUE(map1["height"].isInt()); - EXPECT_EQ(map1["height"], 100); -}