Clean up fastAddProperties and make it more correct#29015
Clean up fastAddProperties and make it more correct#29015dmytrorykun merged 1 commit intofacebook:mainfrom dmytrorykun:fix-fastAddProperties
Conversation
|
Comparing: 7039834...fbcbb40 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
| attributeConfig = validAttributes[propKey]; | ||
|
|
||
| if (attributeConfig === undefined) { | ||
| if (!attributeConfig) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
What I was just mentioning about: we could change the default here and stop ignoring any properties that do not have validAttributes entry.
There was a problem hiding this comment.
That requires the new parser, no? Otherwise, we might get a native exception.
There was a problem hiding this comment.
Update native to ignore unknown props?
This would take us a step closer to removing Static View Configs. We would need this, and a way to specify the bubbling behavior of events. Or we could convert all custom events to be direct, and store a config for which built-in events should bubble (which would also fix the bubbing/direct event name collision bug).
There was a problem hiding this comment.
Update native to ignore unknown props?
This would take us a step closer to removing Static View Configs.
That is already planned!
yungsters
left a comment
yungsters
left a comment
There was a problem hiding this comment.
Consider unit tests, since this logic seems quite nuanced. It's be a good way to also compare the old and new code paths against each other.
| attributeConfig = validAttributes[propKey]; | ||
|
|
||
| if (attributeConfig === undefined) { | ||
| if (!attributeConfig) { |
There was a problem hiding this comment.
I personally prefer the explicitness of attributeConfig == null (which checks for both undefined and null), but this works, too.
| } else if (typeof attributeConfig.process === 'function') { | ||
| newValue = attributeConfig.process(nextProp); | ||
| } else if (typeof attributeConfig.diff === 'function') { | ||
| newValue = nextProp; |
There was a problem hiding this comment.
Consider leaving an explanatory comment here.
| let newValue; | ||
|
|
||
| if (typeof attributeConfig !== 'object') { | ||
| if (!updatePayload) { | ||
| updatePayload = ({}: {[string]: $FlowFixMe}); | ||
| } | ||
| updatePayload[propKey] = nextProp; | ||
| continue; | ||
| if (typeof nextProp === 'function') { | ||
| newValue = (true: any); | ||
| } else if (typeof attributeConfig !== 'object') { | ||
| newValue = nextProp; | ||
| } else if (typeof attributeConfig.process === 'function') { | ||
| newValue = attributeConfig.process(nextProp); | ||
| } else if (typeof attributeConfig.diff === 'function') { | ||
| newValue = nextProp; | ||
| } |
There was a problem hiding this comment.
Could this work?
let newValue = typeof nextProp === 'function' ? true : nextProp;
if (typeof attributeConfig === 'object' && typeof attributeConfig.process === 'function') {
newValue = attributeConfig.process(nextProp);
}
There was a problem hiding this comment.
Could do typeof attributeConfig?.process === 'function', too.
## Summary This PR makes some fixes to the `fastAddProperties` function: - Use `if (!attributeConfig)` instead of `if (attributeConfig === undefined)` to account for `null`. - If a prop has an Object `attributeConfig` with a `diff` function defined on it, treat it as an atomic value to keep the semantics of `diffProperties`. ## How did you test this change? Build and run RNTester app. DiffTrain build for commit b37e4b4.
Completes #65845 which only updated Canary not Experimental. <details> <summary> React upstream changes</summary> - facebook/react#29026 - facebook/react#29025 - facebook/react#28743 - facebook/react#29022 - facebook/react#29023 - facebook/react#29015 - facebook/react#29016 - facebook/react#28988 - facebook/react#28987 - facebook/react#28986 - facebook/react#29014 - facebook/react#28982 - facebook/react#29006 - facebook/react#28973 - facebook/react#28841 - facebook/react#28964 - facebook/react#28990 - facebook/react#29003 - facebook/react#28989 - facebook/react#28893 - facebook/react#28887 - facebook/react#28807 - facebook/react#28978 - facebook/react#28963 - facebook/react#28972 - facebook/react#28970 - facebook/react#28816 - facebook/react#28977 - facebook/react#28974 - facebook/react#28976 - facebook/react#28975 - facebook/react#28969 - facebook/react#28966 - facebook/react#28056 </details>
Completes vercel#65845 which only updated Canary not Experimental. <details> <summary> React upstream changes</summary> - facebook/react#29026 - facebook/react#29025 - facebook/react#28743 - facebook/react#29022 - facebook/react#29023 - facebook/react#29015 - facebook/react#29016 - facebook/react#28988 - facebook/react#28987 - facebook/react#28986 - facebook/react#29014 - facebook/react#28982 - facebook/react#29006 - facebook/react#28973 - facebook/react#28841 - facebook/react#28964 - facebook/react#28990 - facebook/react#29003 - facebook/react#28989 - facebook/react#28893 - facebook/react#28887 - facebook/react#28807 - facebook/react#28978 - facebook/react#28963 - facebook/react#28972 - facebook/react#28970 - facebook/react#28816 - facebook/react#28977 - facebook/react#28974 - facebook/react#28976 - facebook/react#28975 - facebook/react#28969 - facebook/react#28966 - facebook/react#28056 </details>
6 participants
Summary
This PR makes some fixes to the
fastAddPropertiesfunction:if (!attributeConfig)instead ofif (attributeConfig === undefined)to account fornull.attributeConfigwith adifffunction defined on it, treat it as an atomic value to keep the semantics ofdiffProperties.How did you test this change?
Build and run RNTester app.