Add Flag to Favor Hydration Performance over User Safety #28655
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
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: 2ec2aae...b41df97 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
|
rickhanlonii
approved these changes
Mar 27, 2024
|
Small nit, but IMO all flags should be prefixed with |
sebmarkbage
force-pushed
the
favorperf
branch
3 times, most recently
from
f8123f7
to
db0f3b9
Compare
If false, this ignores text comparison checks during hydration at the risk of privacy safety.
github-actions bot
pushed a commit
that referenced
this pull request
Mar 27, 2024
If false, this ignores text comparison checks during hydration at the risk of privacy safety. Since React 18 we recreate the DOM starting from the nearest Suspense boundary if any of the text content mismatches. This ensures that if we have nodes that otherwise line up correctly such as if they're the same type of Component but in a different order, then we don't accidentally transfer state or attributes to the wrong one. If we didn't do this e.g. attributes like image src might not line up with the text. E.g. you might show the wrong profile picture with the wrong name. However, the main reason we do this is because it's a security/privacy concern if state from the original node can transfer to the other one. For example if you start typing into a text field to reply to a story but then it turns out that the hydration was in a different order, you might submit that text into a different story than you intended. Similarly, if you've already clicked an item and that gets replayed using Action replaying or is synchronously force hydrated - that click might end up applying to a different item in the list than you intended. E.g. liking the wrong photo. Unfortunately a common case where this happens is when Google Translate is applied to a page. It'll always cause mismatches and recreate the tree. Most of the time this wouldn't be visible to users because it'd just recreate to the same thing and then translate again. It can affect metrics that trace when this hydration happened though. Meta can use this flag to decide if they favor this perf metric over the risk to user privacy. This is similar to the old enableClientRenderFallbackOnTextMismatch flag except this flag doesn't patch up the text when there's a mismatch. Because we don't have the patching anymore. The assumption is that it is safe to ignore the safety concern because we assume it's a match and therefore favoring not patching it will lead to better perf. DiffTrain build for [5910eb3](5910eb3)
|
It's definitely the right default to favor correctness over performance. But it helps to have a feature flag so that we can compare the performance impact. For example, if we were to see a performance difference it could indicate that metrics might not be capturing the time taken to fix up hydration mismatches, in which case it wouldn't actually be a regression but just an incomplete measurement. Thanks for adding the flag so we can investigate! |
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
) If false, this ignores text comparison checks during hydration at the risk of privacy safety. Since React 18 we recreate the DOM starting from the nearest Suspense boundary if any of the text content mismatches. This ensures that if we have nodes that otherwise line up correctly such as if they're the same type of Component but in a different order, then we don't accidentally transfer state or attributes to the wrong one. If we didn't do this e.g. attributes like image src might not line up with the text. E.g. you might show the wrong profile picture with the wrong name. However, the main reason we do this is because it's a security/privacy concern if state from the original node can transfer to the other one. For example if you start typing into a text field to reply to a story but then it turns out that the hydration was in a different order, you might submit that text into a different story than you intended. Similarly, if you've already clicked an item and that gets replayed using Action replaying or is synchronously force hydrated - that click might end up applying to a different item in the list than you intended. E.g. liking the wrong photo. Unfortunately a common case where this happens is when Google Translate is applied to a page. It'll always cause mismatches and recreate the tree. Most of the time this wouldn't be visible to users because it'd just recreate to the same thing and then translate again. It can affect metrics that trace when this hydration happened though. Meta can use this flag to decide if they favor this perf metric over the risk to user privacy. This is similar to the old enableClientRenderFallbackOnTextMismatch flag except this flag doesn't patch up the text when there's a mismatch. Because we don't have the patching anymore. The assumption is that it is safe to ignore the safety concern because we assume it's a match and therefore favoring not patching it will lead to better perf.
This was referenced Apr 24, 2024
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.
If false, this ignores text comparison checks during hydration at the risk of privacy safety.
Since React 18 we recreate the DOM starting from the nearest Suspense boundary if any of the text content mismatches. This ensures that if we have nodes that otherwise line up correctly such as if they're the same type of Component but in a different order, then we don't accidentally transfer state or attributes to the wrong one.
If we didn't do this e.g. attributes like image src might not line up with the text. E.g. you might show the wrong profile picture with the wrong name. However, the main reason we do this is because it's a security/privacy concern if state from the original node can transfer to the other one. For example if you start typing into a text field to reply to a story but then it turns out that the hydration was in a different order, you might submit that text into a different story than you intended. Similarly, if you've already clicked an item and that gets replayed using Action replaying or is synchronously force hydrated - that click might end up applying to a different item in the list than you intended. E.g. liking the wrong photo.
Unfortunately a common case where this happens is when Google Translate is applied to a page. It'll always cause mismatches and recreate the tree. Most of the time this wouldn't be visible to users because it'd just recreate to the same thing and then translate again. It can affect metrics that trace when this hydration happened though.
Meta can use this flag to decide if they favor this perf metric over the risk to user privacy.
This is similar to the old enableClientRenderFallbackOnTextMismatch flag except this flag doesn't patch up the text when there's a mismatch. Because we don't have the patching anymore. The assumption is that it is safe to ignore the safety concern because we assume it's a match and therefore favoring not patching it will lead to better perf.