Mirror of upstream PR #33186 #41
Open
Conversation
This file contains hidden or 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
This is a fix for a problem where React retains shadow nodes longer than it needs to. The behaviour is shown in React Native test: https://github.com/facebook/react-native/blob/main/packages/react-native/src/private/__tests__/utilities/__tests__/ShadowNodeReferenceCounter-itest.js#L169 # Problem When React commits a new shadow tree, old shadow nodes are stored inside `fiber.alternate.stateNode`. This is not cleared up until React clones the node again. This may be problematic if mutation deletes a subtree, in that case `fiber.alternate.stateNode` will retain entire subtree until next update. In case of image nodes, this means retaining entire images. So when React goes from revision A: `<View><View /></View>` to revision B: `<View />`, `fiber.alternate.stateNode` will be pointing to Shadow Node that represents revision A..  # Fix To fix this, this PR adds a new feature flag `enableEagerAlternateStateNodeCleanup`. When enabled, `alternate.stateNode` is proactively pointed towards finishedWork's stateNode, releasing resources sooner. I have verified this fixes the issue [demonstrated by React Native tests](https://github.com/facebook/react-native/blob/main/packages/react-native/src/private/__tests__/utilities/__tests__/ShadowNodeReferenceCounter-itest.js#L169). All existing React tests pass when the flag is enabled.
…ook#33028) ## Summary Our builds generate files with a `.mjs` file extension. These are currently filtered out by `ReactFlightWebpackPlugin` so I am updating it to support this file extension. This fixes facebook#33155 ## How did you test this change? I built the plugin with this change and used `yalc` to test it in my project. I confirmed the expected files now show up in `react-client-manifest.json`
|
How did you do this? |
…ackTrace (facebook#33143) When we get the source location for "View source for this element" we should be using the enclosing function of the callsite of the child. So that we don't just point to some random line within the component. This is similar to the technique in facebook#33136. This technique is now really better than the fake throw technique, when available. So I now favor the owner technique. The only problem it's only available in DEV and only if it has a child that's owned (and not filtered). We could implement this same technique for the error that's thrown in the fake throwing solution. However, we really shouldn't need that at all because for client components we should be able to call `inspect(fn)` at least in Chrome which is even better.
…3159) This keeps track of the transition lane allocated for this event. I want to be able to use the current one within sync work flushing to know which lane needs its loading indicator cleared. It's also a bit weird that transition work scheduled inside sync updates in the same event aren't entangled with other transitions in that event when `flushSync` is. Therefore this moves it to reset after flushing. It should have no impact. Just splitting it out into a separate PR for an abundance of caution. The only thing this might affect would be if the React internals throws and it doesn't reset after. But really it doesn't really have to reset and they're all entangled anyway.
…onLane (facebook#33188) When we're entangled with an async action lane we use that lane instead of the currentEventTransitionLane. Conversely, if we start a new async action lane we reuse the currentEventTransitionLane. So they're basically supposed to be in sync but they're not if you resolve the async action and then schedule new stuff in the same event. Then you end up with two transitions in the same event with different lanes. By stashing it like this we fix that but it also gives us an opportunity to check just the currentEventTransitionLane to see if this event scheduled any regular Transition updates or Async Transitions.
Stacked on facebook#33159. This implements `onDefaultTransitionIndicator`. The sequence is: 1) In `markRootUpdated` we schedule Transition updates as needing `indicatorLanes` on the root. This tracks the lanes that currently need an indicator to either start or remain going until this lane commits. 2) Track mutations during any commit. We use the same hook that view transitions use here but instead of tracking it just per view transition scope, we also track a global boolean for the whole root. 3) If a sync/default commit had any mutations, then we clear the indicator lane for the `currentEventTransitionLane`. This requires that the lane is still active while we do these commits. See facebook#33159. In other words, a sync update gets associated with the current transition and it is assumed to be rendering the loading state for that corresponding transition so we don't need a default indicator for this lane. 4) At the end of `processRootScheduleInMicrotask`, right before we're about to enter a new "event transition lane" scope, it is no longer possible to render any more loading states for the current transition lane. That's when we invoke `onDefaultTransitionIndicator` for any roots that have new indicator lanes. 5) When we commit, we remove the finished lanes from `indicatorLanes` and once that reaches zero again, then we can clean up the default indicator. This approach means that you can start multiple different transitions while an indicator is still going but it won't stop/restart each time. Instead, it'll wait until all are done before stopping. Follow ups: - [x] Default updates are currently not enough to cancel because those aren't flush in the same microtask. That's unfortunate. facebook#33186 - [x] Handle async actions before the setState. Since these don't necessarily have a root this is tricky. facebook#33190 - [x] Disable for `useDeferredValue`. ~Since it also goes through `markRootUpdated` and schedules a Transition lane it'll get a default indicator even though it probably shouldn't have one.~ EDIT: Turns out this just works because it doesn't go through `markRootUpdated` when work is left behind. - [x] Implement built-in DOM version by default. facebook#33162
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.
Mirrored from facebook/react PR facebook#33186