From 7f1ed6848f89bdccc7f7a5cc76019eec67e76b2f Mon Sep 17 00:00:00 2001
From: Valentin Shergin <shergin@fb.com>
Date: Fri, 8 Jun 2018 20:16:22 -0700
Subject: [PATCH] Fabric: `convertRawProp` was extended to accept an optional
 default value

Summary:
During transforming raw prop (`rawProps[<prop>]`) to typed props (`MyProps.<prop>`) we can face three different cases:
 * `rawProps` collection has proper serialized value for the key. In this case, we have to set a new value of the typed prop converting value using `fromDynamic`.
 * `rawProps` collection does not have value for the key. In this case, we have to copy a value from source prop (`sourceValue`).
 * `rawProps` collection has `null` value for the key. This is the special case which means that the prop was removed from the particular component instance and we have to reset it to some *default* value (which is *not* the same as `sourceValue`). Now the default value of the `defaultValue` (sic!) argument is a default value of the type of the value (which may be different from logical default value).

We didn't handle the last case previously and this caused crashes (and unexpected behavior) because `fromDynamic` often cannot handle `null` value.

And yes, all this mean that we also have to update all `convertRawProp` call sites where logical default values are not equal to type-specific default values. This is a potential error-prone place, especially because now we have to specify logical default values in two places (in a prop declaration and in a parameterized constructor). And seems there is no way to avoid that without performance loss (because both of those places are basically constructors).

My hope is that codegen (where default values are also defined in JavaScript) will help with it eventually.

Reviewed By: fkgozali

Differential Revision: D8247652

fbshipit-source-id: 2cbe65f5f5cccd7a0d34aaa19e385aacebfe8cb1
---
 ReactCommon/fabric/core/propsConversions.h | 30 +++++++++++++++++-----
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/ReactCommon/fabric/core/propsConversions.h b/ReactCommon/fabric/core/propsConversions.h
index f4a25f8dff9e2e..548b88f3de4aa0 100644
--- a/ReactCommon/fabric/core/propsConversions.h
+++ b/ReactCommon/fabric/core/propsConversions.h
@@ -21,33 +21,49 @@ inline void fromDynamic(const folly::dynamic &value, int &result) { result = val
 inline void fromDynamic(const folly::dynamic &value, std::string &result) { result = value.getString(); }
 
 template <typename T>
-inline T convertRawProp(const RawProps &rawProps, const std::string &name, const T &defaultValue) {
+inline T convertRawProp(
+  const RawProps &rawProps,
+  const std::string &name,
+  const T &sourceValue,
+  const T &defaultValue = T()
+) {
   auto &&iterator = rawProps.find(name);
   if (iterator == rawProps.end()) {
-    return defaultValue;
+    return sourceValue;
   }
 
   auto &&value = iterator->second;
+
+  // Special case: `null` always means `the prop was removed, use default value`.
+  if (value.isNull()) {
+    return defaultValue;
+  }
+
   T result;
   fromDynamic(value, result);
   return result;
 }
 
 template <typename T>
-inline static folly::Optional<T> convertRawProp(const RawProps &rawProps, const std::string &name, const folly::Optional<T> &defaultValue) {
+inline static folly::Optional<T> convertRawProp(
+  const RawProps &rawProps,
+  const std::string &name,
+  const folly::Optional<T> &sourceValue,
+  const folly::Optional<T> &defaultValue = {}
+) {
   auto &&iterator = rawProps.find(name);
   if (iterator == rawProps.end()) {
-    return defaultValue;
+    return sourceValue;
   }
 
   auto &&value = iterator->second;
-  T result;
 
-  // Special case for optionals: `null` always means `no value`.
+  // Special case: `null` always means `the prop was removed, use default value`.
   if (value.isNull()) {
-    return {};
+    return defaultValue;
   }
 
+  T result;
   fromDynamic(value, result);
   return result;
 }