Facebook/react-native
1
0
Fork 0
mirror of https://github.com/facebook/react-native.git synced 2025-11-01 09:14:26 +00:00
Code Issues Packages Projects Releases Wiki Activity

Correctly handle null values in dynamic props in prop forwarding

Browse Source 
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
This commit is contained in:
Samuel Susla
2021-02-22 05:53:24 -08:00
committed by Facebook GitHub Bot
parent 1fe5cac52a
commit f303266d69
6 changed files with 127 additions and 82 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ReactCommon/react/renderer/components/legacyviewmanagerinterop/LegacyViewManagerInteropViewProps.cpp
+3 -23
View File
@@ -6,38 +6,18 @@
*/
#include "LegacyViewManagerInteropViewProps.h"
#include <react/renderer/core/DynamicPropsUtilities.h>
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
ReactCommon/react/renderer/core/DynamicPropsUtilities.cpp
+35
View File
@@ -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
ReactCommon/react/renderer/core/DynamicPropsUtilities.h
+20
View File
@@ -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 <folly/dynamic.h>
namespace facebook {
namespace react {
folly::dynamic mergeDynamicProps(
folly::dynamic const &source,
folly::dynamic const &patch);
} // namespace react
} // namespace facebook
ReactCommon/react/renderer/core/ShadowNode.cpp
+3 -3
View File
@@ -7,6 +7,7 @@
#include "ShadowNode.h"
#include "Constants.h"
#include "DynamicPropsUtilities.h"
#include "ShadowNodeFragment.h"
#include <better/small_vector.h>
@@ -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 &>(*props);
castedProps.rawProps = copiedProps;
castedProps.rawProps = mergeDynamicProps(
sourceShadowNode.getProps()->rawProps, props->rawProps);
return props;
}
}
ReactCommon/react/renderer/core/tests/DynamicPropsUtilitiesTest.cpp
+66
View File
@@ -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 <gtest/gtest.h>
#include <react/renderer/core/DynamicPropsUtilities.h>
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());
}
ReactCommon/react/renderer/core/tests/FollyDynamicMergePatchTest.cpp
-56
View File
@@ -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 <memory>
#include <folly/dynamic.h>
#include <glog/logging.h>
#include <gtest/gtest.h>
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);
}