Skip to content

Commit 7f1ed68

Browse files
sherginfacebook-github-bot
authored andcommitted
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
1 parent a32be38 commit 7f1ed68

File tree

1 file changed

+23
-7
lines changed

1 file changed

+23
-7
lines changed

ReactCommon/fabric/core/propsConversions.h

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,33 +21,49 @@ inline void fromDynamic(const folly::dynamic &value, int &result) { result = val
2121
inline void fromDynamic(const folly::dynamic &value, std::string &result) { result = value.getString(); }
2222

2323
template <typename T>
24-
inline T convertRawProp(const RawProps &rawProps, const std::string &name, const T &defaultValue) {
24+
inline T convertRawProp(
25+
const RawProps &rawProps,
26+
const std::string &name,
27+
const T &sourceValue,
28+
const T &defaultValue = T()
29+
) {
2530
auto &&iterator = rawProps.find(name);
2631
if (iterator == rawProps.end()) {
27-
return defaultValue;
32+
return sourceValue;
2833
}
2934

3035
auto &&value = iterator->second;
36+
37+
// Special case: `null` always means `the prop was removed, use default value`.
38+
if (value.isNull()) {
39+
return defaultValue;
40+
}
41+
3142
T result;
3243
fromDynamic(value, result);
3344
return result;
3445
}
3546

3647
template <typename T>
37-
inline static folly::Optional<T> convertRawProp(const RawProps &rawProps, const std::string &name, const folly::Optional<T> &defaultValue) {
48+
inline static folly::Optional<T> convertRawProp(
49+
const RawProps &rawProps,
50+
const std::string &name,
51+
const folly::Optional<T> &sourceValue,
52+
const folly::Optional<T> &defaultValue = {}
53+
) {
3854
auto &&iterator = rawProps.find(name);
3955
if (iterator == rawProps.end()) {
40-
return defaultValue;
56+
return sourceValue;
4157
}
4258

4359
auto &&value = iterator->second;
44-
T result;
4560

46-
// Special case for optionals: `null` always means `no value`.
61+
// Special case: `null` always means `the prop was removed, use default value`.
4762
if (value.isNull()) {
48-
return {};
63+
return defaultValue;
4964
}
5065

66+
T result;
5167
fromDynamic(value, result);
5268
return result;
5369
}

0 commit comments

Comments
 (0)