-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[core][iOS] Drop proxiedProperties prop #22280
Changes from all commits
746f386
0312bbc
ea593a1
e328dc7
aa3d6b8
98154ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -7,12 +7,27 @@ namespace react = facebook::react; | |||||
|
||||||
namespace expo { | ||||||
|
||||||
/** | ||||||
Borrows the props map from the source props and applies the update given in the raw props. | ||||||
*/ | ||||||
std::unordered_map<std::string, folly::dynamic> propsMapFromProps(const ExpoViewProps &sourceProps, const react::RawProps &rawProps) { | ||||||
// Move the contents of the source props map – the source props instance will not be used anymore. | ||||||
std::unordered_map<std::string, folly::dynamic> propsMap = std::move(sourceProps.propsMap); | ||||||
|
||||||
// Iterate over values in the raw props object. | ||||||
// Note that it contains only updated props. | ||||||
rawProps.iterateOverValues([&propsMap](react::RawPropsPropNameHash hash, const char *name, const react::RawValue &value) { | ||||||
std::string propName(name); | ||||||
propsMap[propName] = (folly::dynamic)value; | ||||||
}); | ||||||
|
||||||
return propsMap; | ||||||
} | ||||||
|
||||||
ExpoViewProps::ExpoViewProps(const react::PropsParserContext &context, | ||||||
const ExpoViewProps &sourceProps, | ||||||
const react::RawProps &rawProps) | ||||||
: ViewProps(context, sourceProps, rawProps), | ||||||
proxiedProperties( | ||||||
facebook::react::convertRawProp(context, rawProps, "proxiedProperties", sourceProps.proxiedProperties, {})) { | ||||||
} | ||||||
propsMap(propsMapFromProps(sourceProps, rawProps)) {} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
not pretty sure whether it's correct. if yes, nice to have a move semantic here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, i was wrong for this, the move is not necessary because it's already temporary object. |
||||||
|
||||||
} // namespace expo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kudo do you think the move here is safe? As far as I know, the source props will not be used anymore and the
propsMap
is used only once per the lifetime of theExpoViewProps
instance 🤔 We could do a copy if you're not sure.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at first glance, it's strange to me because there's
const ExpoViewProps &sourceProps
in the function declaration and we won't like to mutate the sourceProps. i think it's from react-native to passsourceProps
as a const reference. however, since thepropsMaps
is controlled by us. that's somehow reasonable. let's keep it as-is.