[core][iOS] Drop proxiedProperties prop #22280
Conversation
tsapeta
force-pushed
the
@tsapeta/ios/remove-proxied-properties
branch
from
a1a2504
to
391600e
Compare
expo-bot
added
bot: passed checks
tsapeta
changed the title
| for eventName in eventNames { | ||
| if let viewManager = moduleHolder?.viewManager { | ||
| for prop in viewManager.props { | ||
| // `id` allows every type to be passed in |
There was a problem hiding this comment.
it is not clear to me what the id is. could you explain more details?
There was a problem hiding this comment.
React Native puts some Objective-C types (e.g. NSString, NSNumber, BOOL) as the values in the propTypes of the view config, which are then used to create a method's signature of the prop setter and to know how to convert these props.
See RCTModuleMethod.mm and RCTConvert.m where id: selector just returns itself and leaves the value unconverted.
It's basically the same as here
where we had to describeproxiedProperties as NSDictionary.
| proxiedProperties( | ||
| facebook::react::convertRawProp(context, rawProps, "proxiedProperties", sourceProps.proxiedProperties, {})) { | ||
| } | ||
| propsMap(propsMapFromProps(sourceProps, rawProps)) {} |
There was a problem hiding this comment.
| propsMap(propsMapFromProps(sourceProps, rawProps)) {} | |
| propsMap(std::move(propsMapFromProps(sourceProps, rawProps))) {} |
not pretty sure whether it's correct. if yes, nice to have a move semantic here.
There was a problem hiding this comment.
Since this object is already temporary, doing a move would be redundant and slower
There was a problem hiding this comment.
yep, i was wrong for this, the move is not necessary because it's already temporary object.
| */ | ||
| 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); |
There was a problem hiding this comment.
@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 the ExpoViewProps instance 🤔 We could do a copy if you're not sure.
There was a problem hiding this comment.
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 pass sourceProps as a const reference. however, since the propsMaps is controlled by us. that's somehow reasonable. let's keep it as-is.
tsapeta
force-pushed
the
@tsapeta/ios/remove-proxied-properties
branch
from
3d1a63d
to
98154ea
Compare
# Why This a follow-up to the #22280. # How So, I manually exported all the properties defined in the view component through the view manager. It's interesting to note that the same code seems to work fine with fabric without any changes made to the `ExpoViewProps` class. I don't know much about fabric components, so there might be something important that I'm missing here. But for now, let's go ahead with merging it. I'll look into it further to figure out why it's working with fabric as well. # Test Plan - bare-expo ✅ - fabric-tester ✅
# Why This a follow-up to the #22280. # How So, I manually exported all the properties defined in the view component through the view manager. It's interesting to note that the same code seems to work fine with fabric without any changes made to the `ExpoViewProps` class. I don't know much about fabric components, so there might be something important that I'm missing here. But for now, let's go ahead with merging it. I'll look into it further to figure out why it's working with fabric as well. # Test Plan - bare-expo ✅ - fabric-tester ✅
Why
Expo native component's props are scoped under single
proxiedPropertiesprop. This has many downsides and we would rather get rid of it and pass all props as they are, without scoping.How
On the old architecture:
propTypesin the view config so React Native doesn't error when it validates the propsproxiedPropertiessetter, I hooked into thesetProps(_:forView:)function in theComponentDataobject which is called for each props updateOn the new architecture:
proxiedPropertiesprop,ExpoViewPropswill be iterating over all props and adding them to a C++ map with prop values stored asfolly::dynamicsourceProps) to the new object and then overridden with the updated props (rawProps)ExpoFabricView(Swift)Note that on the JavaScript side,
proxiedPropertiesare still passed to the native view on Android platform. This will be implemented separately.Also updated the test that depended upon this prop.
Test Plan