Add enableShallowPropDiffing feature flag #29664
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
dmytrorykun
changed the title
|
Comparing: 1df34bd...3a1eb45 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
|
There was a problem hiding this comment.
With enableShallowPropDiffing hardcoded to true
If those test fail, then we need to make sure there is a testing variant that runs then, so we can add the @gate !enableShallowPropDiffing to disable those tests when this flag is on. As it stands, none of the tests seem to be running with this flag enabled, so there's no coverage for it.
dmytrorykun
force-pushed
the
add-enableShallowPropDiffing-feature-flag
branch
from
aa86496
to
f7a28af
Compare
|
@dmytrorykun sorry I wasn't clear, just adding the gate pragma isn't sufficient. The issue is that there are no test variants running with the feature flag enabled. If you hardcode it to true, you'll see this test failure: 
This shows that one of the tests you gated actually passes when the flag is enabled (which we fail for, gate tests must fail if the condition isn't met). This doesn't fail on the PR, because the test is not running when this flag is enabled. I can help creating a test variant that runs when the variant is enabled. |
|
PR for xplat test variants: #29734 |
rickhanlonii
added a commit
that referenced
this pull request
Jun 4, 2024
## Overview We didn't have any tests that ran in persistent mode with the xplat feature flags (for either variant). As a result, invalid test gating like in #29664 were not caught. This PR adds test flavors for `ReactFeatureFlag-native-fb.js` in both variants.
…leShallowPropDiffing is set to true.
dmytrorykun
force-pushed
the
add-enableShallowPropDiffing-feature-flag
branch
from
be05d6f
to
3a1eb45
Compare
rickhanlonii
approved these changes
Jun 5, 2024
github-actions bot
pushed a commit
that referenced
this pull request
Jun 5, 2024
## Summary We currently do deep diffing for object props, and also use custom differs, if they are defined, for props with custom attribute config. The idea is to simply do a `===` comparison instead of all that work. We will do less computation on the JS side, but send more data to native. The hypothesis is that this change should be neutral in terms of performance. If that's the case, we'll be able to get rid of custom differs, and be one step closer to deleting view configs. This PR adds the `enableShallowPropDiffing` feature flag to support this experiment. ## How did you test this change? With `enableShallowPropDiffing` hardcoded to `true`: ``` yarn test packages/react-native-renderer ``` This fails on the following test cases: - should use the diff attribute - should do deep diffs of Objects by default - should skip deeply-nested changed functions Which makes sense with this change. These test cases should be deleted if the experiment is shipped. DiffTrain build for [eb259b5](eb259b5)
github-actions bot
pushed a commit
that referenced
this pull request
Jun 5, 2024
## Summary We currently do deep diffing for object props, and also use custom differs, if they are defined, for props with custom attribute config. The idea is to simply do a `===` comparison instead of all that work. We will do less computation on the JS side, but send more data to native. The hypothesis is that this change should be neutral in terms of performance. If that's the case, we'll be able to get rid of custom differs, and be one step closer to deleting view configs. This PR adds the `enableShallowPropDiffing` feature flag to support this experiment. ## How did you test this change? With `enableShallowPropDiffing` hardcoded to `true`: ``` yarn test packages/react-native-renderer ``` This fails on the following test cases: - should use the diff attribute - should do deep diffs of Objects by default - should skip deeply-nested changed functions Which makes sense with this change. These test cases should be deleted if the experiment is shipped. DiffTrain build for commit eb259b5.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
We currently do deep diffing for object props, and also use custom differs, if they are defined, for props with custom attribute config.
The idea is to simply do a
===comparison instead of all that work. We will do less computation on the JS side, but send more data to native.The hypothesis is that this change should be neutral in terms of performance. If that's the case, we'll be able to get rid of custom differs, and be one step closer to deleting view configs.
This PR adds the
enableShallowPropDiffingfeature flag to support this experiment.How did you test this change?
With
enableShallowPropDiffinghardcoded totrue:This fails on the following test cases:
Which makes sense with this change. These test cases should be deleted if the experiment is shipped.