Skip to content
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

Clean up fastAddProperties and make it more correct #29015

Merged
merged 1 commit into from
May 8, 2024

Conversation

dmytrorykun
Copy link
Contributor

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.

@react-sizebot
Copy link

react-sizebot commented May 7, 2024

Comparing: 7039834...fbcbb40

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.66 kB 6.66 kB = 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 494.06 kB 494.06 kB = 88.22 kB 88.22 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 498.86 kB 498.86 kB = 88.93 kB 88.93 kB
facebook-www/ReactDOM-prod.classic.js = 591.22 kB 591.22 kB = 103.96 kB 103.96 kB
facebook-www/ReactDOM-prod.modern.js = 567.44 kB 567.44 kB = 100.36 kB 100.36 kB
test_utils/ReactAllWarnings.js Deleted 64.26 kB 0.00 kB Deleted 16.02 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
test_utils/ReactAllWarnings.js Deleted 64.26 kB 0.00 kB Deleted 16.02 kB 0.00 kB

Generated by 🚫 dangerJS against fbcbb40

Comment on lines 463 to 467
attributeConfig = validAttributes[propKey];

if (attributeConfig === undefined) {
if (!attributeConfig) {
continue;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I was just mentioning about: we could change the default here and stop ignoring any properties that do not have validAttributes entry.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That requires the new parser, no? Otherwise, we might get a native exception.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update native to ignore unknown props?
This would take us a step closer to removing Static View Configs.

That is already planned!

Copy link
Contributor

@yungsters yungsters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -462,27 +462,27 @@ function fastAddProperties(

attributeConfig = validAttributes[propKey];

if (attributeConfig === undefined) {
if (!attributeConfig) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider leaving an explanatory comment here.

Comment on lines 469 to 483
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;
}
Copy link
Member

@javache javache May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this work?

    let newValue = typeof nextProp === 'function' ? true : nextProp;
    if (typeof attributeConfig === 'object' && typeof attributeConfig.process === 'function') {
      newValue = attributeConfig.process(nextProp);
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could do typeof attributeConfig?.process === 'function', too.

@dmytrorykun dmytrorykun merged commit b37e4b4 into facebook:main May 8, 2024
38 checks passed
github-actions bot pushed a commit that referenced this pull request May 8, 2024
## 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants