-
Notifications
You must be signed in to change notification settings - Fork 47k
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
[Transition Tracing] onMarkerIncomplete - Tracing Marker/Suspense Boundary Deletions #24885
Conversation
Comparing: 19e9a4c...0cbae60 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
|
d029eed
to
cf03c77
Compare
ddc1ba6
to
ee037cb
Compare
@@ -974,11 +974,15 @@ function updateTracingMarkerComponent( | |||
// or updated it, so we can create a new set of transitions each time | |||
if (current === null) { | |||
const currentTransitions = getPendingTransitions(); | |||
// TODO: What if the parent markers change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would that happen
markerIncomplete.forEach(({transitions, deletions}, markerName) => { | ||
transitions.forEach(transition => { | ||
const filteredDeletions = []; | ||
deletions.forEach(deletion => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this deletions
array exist? Why can't we schedule onMarkerIncomplete
directly from the deletion effect?
@@ -148,6 +211,9 @@ export function pushRootMarkerInstance(workInProgress: Fiber): void { | |||
const markerInstance: TracingMarkerInstance = { | |||
transitions: new Set([transition]), | |||
pendingBoundaries: null, | |||
deletions: null, | |||
parents: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a waste of a field, can use return
path
marker.deletions.push({ | ||
type: 'marker', | ||
name: deletedFiber.memoizedProps.name, | ||
transitions: instance.transitions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this probably
1fc184d
to
bcd87ff
Compare
|
||
const markerInstance = fiber.stateNode; | ||
const markerTransitions = markerInstance.transitions; | ||
const abortMarker = Array.from(transitions).some(transition => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you converting this to an array? Seems like the only thing you're doing is iterating over it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, isn't it an array already? Array.from
does a copy, which is unnecessary in this case and generally something to try to avoid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a set actually but I'm just going to use forEach
to loop over it since there's probably max a couple of entries
let fiber = deletedFiber; | ||
let isInDeletedTree = true; | ||
while (fiber !== null) { | ||
const instance = deletedFiber.stateNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be hoisted outside of the loop. Also make the name more specific since there's a marker instance variable below, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type should also be refined. What kind of instance is it?
markerTransitions.has(transition), | ||
); | ||
|
||
if (abortMarker) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do a normal for loop, you can put all this code inside the loop and then break
out of it once you find a match. We tend to prefer that when possible instead of the functional array methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't because transitions is a set
markerInstance.pendingBoundaries.delete(instance); | ||
|
||
addMarkerProgressCallbackToPendingTransition( | ||
markerInstance.name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can get this from the fiber prop
case TracingMarkerComponent: | ||
const transitions = deletedFiberInstance.transitions; | ||
|
||
const markerInstance = fiber.stateNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Add type annotation
const markerTransitions = markerInstance.transitions; | ||
if (markerTransitions !== null && transitions !== null) { | ||
let abortMarker = false; | ||
transitions.forEach(transition => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment what's going on here
}); | ||
|
||
if (abortMarker) { | ||
if (markerInstance.aborts === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this into the forEach
.
Extract this logic to abortTransitionMarker
nearestMountedAncestor: Fiber, | ||
abort: TransitionAbort, | ||
) { | ||
const deletedFiberInstance: OffscreenInstance = deletedFiber.stateNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explain what this is doing
'finding all pending markers waiting on suspense boundary and cancels them etc.'
|
||
if (enableTransitionTracing) { | ||
// We need to mark this fiber's parents as deleted | ||
const instance: OffscreenInstance = deletedFiber.stateNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to the suspense boundary code
@@ -3023,14 +3165,16 @@ function commitTracingMarkerPassiveMountEffect(finishedWork: Fiber) { | |||
(instance.pendingBoundaries === null || | |||
instance.pendingBoundaries.size === 0) | |||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move all this code to the Suspense offscreen instance instead of scheduling a passive effect on the tracing marker?
@@ -2987,6 +3134,11 @@ function commitOffscreenPassiveMountEffects( | |||
} | |||
|
|||
commitTransitionProgress(finishedWork); | |||
|
|||
if (!isHidden) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment here TODO this will be in an else branch
@@ -120,6 +159,28 @@ export function processTransitionCallbacks( | |||
} | |||
} | |||
|
|||
function getFilteredDeletion(abort: TransitionAbort, endTime: number) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: inline
}; | ||
workInProgress.stateNode = markerInstance; | ||
|
||
workInProgress.flags |= Passive; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commet this
) { | ||
const markerInstance: TracingMarkerInstance = currentFiber.stateNode; | ||
const deletedInstance: OffscreenInstance = deletedFiber.stateNode; | ||
const deletedTransitions = deletedFiber.stateNode.transitions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deletedInstance
} | ||
|
||
aborts.push(abort); | ||
addMarkerIncompleteCallbackToPendingTransition( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to call this if abort isn't null
if ( | ||
nearestMountedAncestor.deletions !== null && | ||
nearestMountedAncestor.deletions.includes(fiber) | ||
) { | ||
isInDeletedTree = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need this maybe return = null.
Split in two.
transitions, | ||
pendingBoundaries, | ||
); | ||
|
||
if (pendingBoundaries.size === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commet nothing's waiting
b105975
to
b4698c8
Compare
* 'main' of ssh://github.com/GrinZero/react: (26 commits) [devtools][easy] Fix flow type (facebook#25147) Remove Symbol Polyfill (again) (facebook#25144) Remove ReactFiberFlags MountLayoutDev and MountPassiveDev (facebook#25091) experimental_use(promise) (facebook#25084) [Transition Tracing] onMarkerIncomplete - Tracing Marker/Suspense Boundary Deletions (facebook#24885) [Flight] Add support for Webpack Async Modules (facebook#25138) Fix typo: supportsMicrotask -> supportsMicrotasks (facebook#25142) Allow functions to be used as module references (facebook#25137) Test the node-register hooks in unit tests (facebook#25132) Return closestInstance in `getInspectorDataForViewAtPoint` (facebook#25118) [DevTools] Highlight RN elements on hover (facebook#25106) Update fixtures/flight to webpack 5 (facebook#25115) Align StrictMode behaviour with production (facebook#25049) Scaffolding for useMemoCache hook (facebook#25123) devtools: Fix typo from directores to directories (facebook#25124) fixture: Fix typo from perfomrance to performance (facebook#25100) [DevTools] Add events necessary for click to inspect on RN (facebook#25111) Add missing createServerContext for experimental shared subset (facebook#25114) support subresource integrity for bootstrapScripts and bootstrapModules (facebook#25104) make preamble and postamble types explicit and fix typo (facebook#25102) ...
Summary: This sync includes the following changes: - **[c28f313e6](facebook/react@c28f313e6 )**: experimental_use(promise) for SSR ([#25214](facebook/react#25214)) //<Andrew Clark>// - **[d6f9628a8](facebook/react@d6f9628a8 )**: Remove some RSC subset entry points that were removed in the main entry point ([#25209](facebook/react#25209)) //<Sebastian Markbåge>// - **[a473d08fc](facebook/react@a473d08fc )**: Update to Flow from 0.97 to 0.122 ([#25204](facebook/react#25204)) //<Jan Kassens>// - **[7028ce745](facebook/react@7028ce745 )**: experimental_use(promise) for Server Components ([#25207](facebook/react#25207)) //<Andrew Clark>// - **[bfb65681e](facebook/react@bfb65681e )**: experimental_use(context)([#25202](facebook/react#25202)) //<mofeiZ>// - **[f0efa1164](facebook/react@f0efa1164 )**: [flow] remove custom suppress comment config ([#25170](facebook/react#25170)) //<Jan Kassens>// - **[2e7f422fe](facebook/react@2e7f422fe )**: Refactor: its type is Container ([#25153](facebook/react#25153)) //<bubucuo>// - **[2c2d9a1df](facebook/react@2c2d9a1df )**: [eslint-plugin-react-hooks] only allow capitalized component names ([#25162](facebook/react#25162)) //<Jan Kassens>// - **[36c908a6c](facebook/react@36c908a6c )**: Don't use the Flight terminology in public error messages ([#25166](facebook/react#25166)) //<Sebastian Markbåge>// - **[8d1b057ec](facebook/react@8d1b057ec )**: [Flight] Minor error handling fixes ([#25151](facebook/react#25151)) //<Sebastian Markbåge>// - **[9ff738f53](facebook/react@9ff738f53 )**: [devtools][easy] Fix flow type ([#25147](facebook/react#25147)) //<Tianyu Yao>// - **[0de3ddf56](facebook/react@0de3ddf56 )**: Remove Symbol Polyfill (again) ([#25144](facebook/react#25144)) //<Ricky>// - **[b36f72235](facebook/react@b36f72235 )**: Remove ReactFiberFlags MountLayoutDev and MountPassiveDev ([#25091](facebook/react#25091)) //<Samuel Susla>// - **[b6978bc38](facebook/react@b6978bc38 )**: experimental_use(promise) ([#25084](facebook/react#25084)) //<Andrew Clark>// - **[11ed7010c](facebook/react@11ed7010c )**: [Transition Tracing] onMarkerIncomplete - Tracing Marker/Suspense Boundary Deletions ([#24885](facebook/react#24885)) //<Luna Ruan>// - **[b79894259](facebook/react@b79894259 )**: [Flight] Add support for Webpack Async Modules ([#25138](facebook/react#25138)) //<Sebastian Markbåge>// - **[c8b778b7f](facebook/react@c8b778b7f )**: Fix typo: supportsMicrotask -> supportsMicrotasks ([#25142](facebook/react#25142)) //<kwzr>// - **[d0f396651](facebook/react@d0f396651 )**: Allow functions to be used as module references ([#25137](facebook/react#25137)) //<Sebastian Markbåge>// - **[38c5d8a03](facebook/react@38c5d8a03 )**: Test the node-register hooks in unit tests ([#25132](facebook/react#25132)) //<Sebastian Markbåge>// - **[3f70e68ce](facebook/react@3f70e68ce )**: Return closestInstance in `getInspectorDataForViewAtPoint` ([#25118](facebook/react#25118)) //<Tianyu Yao>// - **[3d443cad7](facebook/react@3d443cad7 )**: Update fixtures/flight to webpack 5 ([#25115](facebook/react#25115)) //<Tim Neutkens>// - **[5d1ce6513](facebook/react@5d1ce6513 )**: Align StrictMode behaviour with production ([#25049](facebook/react#25049)) //<Samuel Susla>// - **[9e67e7a31](facebook/react@9e67e7a31 )**: Scaffolding for useMemoCache hook ([#25123](facebook/react#25123)) //<Joseph Savona>// - **[19e9a4c68](facebook/react@19e9a4c68 )**: Add missing createServerContext for experimental shared subset ([#25114](facebook/react#25114)) //<Jiachi Liu>// - **[6ef466c68](facebook/react@6ef466c68 )**: make preamble and postamble types explicit and fix typo ([#25102](facebook/react#25102)) //<Josh Story>// - **[796d31809](facebook/react@796d31809 )**: Implement basic stylesheet Resources for react-dom ([#25060](facebook/react#25060)) //<Josh Story>// - **[32baab38f](facebook/react@32baab38f )**: [Transition Tracing] Add Tag Field to Marker Instance ([#25085](facebook/react#25085)) //<Luna Ruan>// - **[8ef3a7c08](facebook/react@8ef3a7c08 )**: Resume immediately pinged fiber without unwinding ([#25074](facebook/react#25074)) //<Andrew Clark>// - **[7bcc68772](facebook/react@7bcc68772 )**: Remove argument committedLanes from reappearLayoutEffects and recursivelyTraverseReappearLayoutEffects ([#25080](facebook/react#25080)) //<Samuel Susla>// - **[ca990e9a7](facebook/react@ca990e9a7 )**: Add API to force Scheduler to yield for macrotask ([#25044](facebook/react#25044)) //<Andrew Clark>// - **[b4204ede6](facebook/react@b4204ede6 )**: Clean up unused Deletion flag ([#24992](facebook/react#24992)) //<Andrew Clark>// - **[e193be87e](facebook/react@e193be87e )**: [Transition Tracing] Add Offscreen Test ([#25035](facebook/react#25035)) //<Luna Ruan>// - **[9fcaf88d5](facebook/react@9fcaf88d5 )**: Remove rootContainerInstance from unnecessary places ([#25024](facebook/react#25024)) //<Sebastian Markbåge>// - **[80f3d8819](facebook/react@80f3d8819 )**: Mount/unmount passive effects when Offscreen visibility changes ([#24977](facebook/react#24977)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 4ea064e...c28f313 Reviewed By: rickhanlonii Differential Revision: D39384898 fbshipit-source-id: 20b080a53851d6dd9d522c8468dd02aab9ba76db
…ndary Deletions (#24885) This PR adds the `onMarkerIncomplete` callback for tracing marker name changes. Specifically, this PR: * Adds the `onMarkerIncomplete` callback * When a tracing marker is deleted, call `onMarkerIncomplete` with the `name` of the tracing marker for the tracing marker. * When a tracing marker/suspense boundary is deleted, call `onMarkerIncomplete` for every parent tracing marker with the `name` of the tracing marker that caused the transition to be incomplete. * Don't call `onTransitionComplete` or `onMarkerComplete` when `onMarkerIncomplete` is called for all tracing markers with the same transitions, but continue to call `onTransitionProgress`
Summary: This sync includes the following changes: - **[c28f313e6](facebook/react@c28f313e6 )**: experimental_use(promise) for SSR ([facebook#25214](facebook/react#25214)) //<Andrew Clark>// - **[d6f9628a8](facebook/react@d6f9628a8 )**: Remove some RSC subset entry points that were removed in the main entry point ([facebook#25209](facebook/react#25209)) //<Sebastian Markbåge>// - **[a473d08fc](facebook/react@a473d08fc )**: Update to Flow from 0.97 to 0.122 ([facebook#25204](facebook/react#25204)) //<Jan Kassens>// - **[7028ce745](facebook/react@7028ce745 )**: experimental_use(promise) for Server Components ([facebook#25207](facebook/react#25207)) //<Andrew Clark>// - **[bfb65681e](facebook/react@bfb65681e )**: experimental_use(context)([facebook#25202](facebook/react#25202)) //<mofeiZ>// - **[f0efa1164](facebook/react@f0efa1164 )**: [flow] remove custom suppress comment config ([facebook#25170](facebook/react#25170)) //<Jan Kassens>// - **[2e7f422fe](facebook/react@2e7f422fe )**: Refactor: its type is Container ([facebook#25153](facebook/react#25153)) //<bubucuo>// - **[2c2d9a1df](facebook/react@2c2d9a1df )**: [eslint-plugin-react-hooks] only allow capitalized component names ([facebook#25162](facebook/react#25162)) //<Jan Kassens>// - **[36c908a6c](facebook/react@36c908a6c )**: Don't use the Flight terminology in public error messages ([facebook#25166](facebook/react#25166)) //<Sebastian Markbåge>// - **[8d1b057ec](facebook/react@8d1b057ec )**: [Flight] Minor error handling fixes ([facebook#25151](facebook/react#25151)) //<Sebastian Markbåge>// - **[9ff738f53](facebook/react@9ff738f53 )**: [devtools][easy] Fix flow type ([facebook#25147](facebook/react#25147)) //<Tianyu Yao>// - **[0de3ddf56](facebook/react@0de3ddf56 )**: Remove Symbol Polyfill (again) ([facebook#25144](facebook/react#25144)) //<Ricky>// - **[b36f72235](facebook/react@b36f72235 )**: Remove ReactFiberFlags MountLayoutDev and MountPassiveDev ([facebook#25091](facebook/react#25091)) //<Samuel Susla>// - **[b6978bc38](facebook/react@b6978bc38 )**: experimental_use(promise) ([facebook#25084](facebook/react#25084)) //<Andrew Clark>// - **[11ed7010c](facebook/react@11ed7010c )**: [Transition Tracing] onMarkerIncomplete - Tracing Marker/Suspense Boundary Deletions ([facebook#24885](facebook/react#24885)) //<Luna Ruan>// - **[b79894259](facebook/react@b79894259 )**: [Flight] Add support for Webpack Async Modules ([facebook#25138](facebook/react#25138)) //<Sebastian Markbåge>// - **[c8b778b7f](facebook/react@c8b778b7f )**: Fix typo: supportsMicrotask -> supportsMicrotasks ([facebook#25142](facebook/react#25142)) //<kwzr>// - **[d0f396651](facebook/react@d0f396651 )**: Allow functions to be used as module references ([facebook#25137](facebook/react#25137)) //<Sebastian Markbåge>// - **[38c5d8a03](facebook/react@38c5d8a03 )**: Test the node-register hooks in unit tests ([facebook#25132](facebook/react#25132)) //<Sebastian Markbåge>// - **[3f70e68ce](facebook/react@3f70e68ce )**: Return closestInstance in `getInspectorDataForViewAtPoint` ([facebook#25118](facebook/react#25118)) //<Tianyu Yao>// - **[3d443cad7](facebook/react@3d443cad7 )**: Update fixtures/flight to webpack 5 ([facebook#25115](facebook/react#25115)) //<Tim Neutkens>// - **[5d1ce6513](facebook/react@5d1ce6513 )**: Align StrictMode behaviour with production ([facebook#25049](facebook/react#25049)) //<Samuel Susla>// - **[9e67e7a31](facebook/react@9e67e7a31 )**: Scaffolding for useMemoCache hook ([facebook#25123](facebook/react#25123)) //<Joseph Savona>// - **[19e9a4c68](facebook/react@19e9a4c68 )**: Add missing createServerContext for experimental shared subset ([facebook#25114](facebook/react#25114)) //<Jiachi Liu>// - **[6ef466c68](facebook/react@6ef466c68 )**: make preamble and postamble types explicit and fix typo ([facebook#25102](facebook/react#25102)) //<Josh Story>// - **[796d31809](facebook/react@796d31809 )**: Implement basic stylesheet Resources for react-dom ([facebook#25060](facebook/react#25060)) //<Josh Story>// - **[32baab38f](facebook/react@32baab38f )**: [Transition Tracing] Add Tag Field to Marker Instance ([facebook#25085](facebook/react#25085)) //<Luna Ruan>// - **[8ef3a7c08](facebook/react@8ef3a7c08 )**: Resume immediately pinged fiber without unwinding ([facebook#25074](facebook/react#25074)) //<Andrew Clark>// - **[7bcc68772](facebook/react@7bcc68772 )**: Remove argument committedLanes from reappearLayoutEffects and recursivelyTraverseReappearLayoutEffects ([facebook#25080](facebook/react#25080)) //<Samuel Susla>// - **[ca990e9a7](facebook/react@ca990e9a7 )**: Add API to force Scheduler to yield for macrotask ([facebook#25044](facebook/react#25044)) //<Andrew Clark>// - **[b4204ede6](facebook/react@b4204ede6 )**: Clean up unused Deletion flag ([facebook#24992](facebook/react#24992)) //<Andrew Clark>// - **[e193be87e](facebook/react@e193be87e )**: [Transition Tracing] Add Offscreen Test ([facebook#25035](facebook/react#25035)) //<Luna Ruan>// - **[9fcaf88d5](facebook/react@9fcaf88d5 )**: Remove rootContainerInstance from unnecessary places ([facebook#25024](facebook/react#25024)) //<Sebastian Markbåge>// - **[80f3d8819](facebook/react@80f3d8819 )**: Mount/unmount passive effects when Offscreen visibility changes ([facebook#24977](facebook/react#24977)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 4ea064e...c28f313 Reviewed By: rickhanlonii Differential Revision: D39384898 fbshipit-source-id: 20b080a53851d6dd9d522c8468dd02aab9ba76db
This PR adds the
onMarkerIncomplete
callback for tracing marker name changes. Specifically, this PR:onMarkerIncomplete
callbackonMarkerIncomplete
with thename
of the tracing marker for the tracing marker.onMarkerIncomplete
for every parent tracing marker with thename
of the tracing marker that caused the transition to be incomplete.onTransitionComplete
oronMarkerComplete
whenonMarkerIncomplete
is called for all tracing markers with the same transitions, but continue to callonTransitionProgress