Bailout without entering work loop for roots without work WIP #17267
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
This addresses an edge case where React currently does a no-op render and commits. This commit is unnecessary, and can confuse the DevTools Profiler.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit a14fdcf:
|
Size changes (experimental)ReactDOM: size: 0.0%, gzip: -0.0% Details of bundled changes.Comparing: 62ef250...a14fdcf react-dom
react-native-renderer
react-test-renderer
react-art
react-reconciler
|
Size changes (stable)ReactDOM: size: 0.0%, gzip: -0.2% Details of bundled changes.Comparing: 62ef250...a14fdcf react-reconciler
react-dom
react-art
react-test-renderer
react-native-renderer
|
|
Ping~ |
|
Ping @sebmarkbage / @acdlite Seems like this is still worth landing, no? I'd be happy to rebase and fix conflicts, but are there any other blockers? |
|
I think this PR should remain open until reviewed. |
|
Let's merge this PR! React dev tools remains to be broken #16980 |
|
Closing as per this comment: #16980 (comment) |
|
Alternate #22745 |
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.
Wraps up #16980
This addresses some edge cases where React currently does a no-op render and an empty/unnecessary commit.
act().The tests listed below trigger this new early bailout code. Everyone is in
performSyncWorkOnRoot(none trigger the new code inperformConcurrentWorkOnRoot). I spot checked a couple of them to see why the new code is being hit, to see if it looked problematic. Below is my findings:ReactES6Class-test"renders only once when setting state in componentWillMount"This one calls
performSyncWorkOnRoot()twice. The first time is the callback passed fromlegacyRenderSubtreeIntoContainertounbatchedUpdates. The second one (the one that bails out) is whenunbatchedUpdatescallsflushSyncCallbackQueue. This flush can be bailed out on.RaectDOMInput-test"should control a value in reentrant events"This one hits the new codepath when it dispatch a discrete "input" event. It looks like when our test calls
node.dispatchEvent()for the "input" event, something is actually dispatching a series of events (input, input, blur, focus) which causes more updates to be scheduled with React than necessary. Now we bail out after the first.ReactCompositeComponent"should warn aboutsetStatein render"This one calls
setStatein render. Without this call, the bailout codepath doesn't get hit. Looks like thesetStatecall leaves two things in the queue, so when the subsequent call toflushSyncCallbackQueueflushes them both, the second one is a no-op. The first thing gets added to the queue when thesetStatecall is made. The second one bycommitRootImpl()when it callsensureRootIsScheduled()becausegetNextRootExpirationTimeToWorkOnreturns a value that indicates there's more work.ReactTestUtils.act()> legacy mode › sync › flushes effects on every callReactTestUtils.act()> blocking mode › sync › flushes effects on every callReactDOMInput> should control a value in reentrant eventsReactDOMInput> should control values in reentrant events with different targetsReactDOMInput> switching text inputs between numeric and string numbers › changes the number 2 to "2.0" using a change handlerReactDOMInput> should control radio buttons if the tree updates during renderReactDOMInput> assigning the value attribute on controlled inputs › always sets the attribute when values change on text inputsReactDOMInput> assigning the value attribute on controlled inputs › does not set the value attribute on number inputs if focusedReactDOMInput> assigning the value attribute on controlled inputs › sets the value attribute on number inputs on blurReactDOMInput> setting a controlled input to undefined › reverts the value attribute to the initial valueReactDOMInput> setting a controlled input to undefined › preserves the value propertyReactDOMInput> setting a controlled input to null › reverts the value attribute to the initial valueReactDOMInput> setting a controlled input to null › preserves the value propertyReactUpdates> should queue updates from during mountReactUpdates> uses correct base state for setState inside render phaseReactFresh> can preserve state for forwardRefReactFresh> should not consider two forwardRefs around the same type to be equivalentReactFresh> can update forwardRef render function with its wrapperReactFresh> can update forwardRef render function in isolationReactFresh> can preserve state for simple memoReactFresh> can preserve state for memo with custom comparisonReactFresh> can update simple memo function in isolationReactFresh> can preserve state for memo(forwardRef)ReactFresh> can preserve state for lazy after resolutionReactFresh> can patch lazy before resolutionReactFresh> can patch lazy(forwardRef) before resolutionReactFresh> can patch lazy(memo) before resolutionReactFresh> can patch lazy(memo(forwardRef)) before resolutionReactFresh> can patch both trees while suspense is displaying the fallbackReactFresh> does not re-render ancestor components unnecessarily during a hot updateReactFresh> does not leak state between componentsReactFresh> can force remount by changing signatureReactFresh> can remount on signature change within a wrapperReactFresh> can remount on signature change within a simple memo wrapperReactFresh> can remount on signature change within a lazy simple memo wrapperReactFresh> can remount on signature change within forwardRefReactFresh> can remount on signature change within forwardRef render functionReactFresh> can remount on signature change within nested memoReactFresh> can remount on signature change within a memo wrapper and custom comparisonReactFresh> can remount on signature change within a classReactFresh> can remount on signature change within a context providerReactFresh> can remount on signature change within a context consumerReactFresh> can remount on signature change within a suspense nodeReactFresh> can remount on signature change within a mode nodeReactFresh> can remount on signature change within a fragment nodeReactFresh> can remount on signature change within multiple siblingsReactFresh> can remount on signature change within a profiler nodeReactFresh> resets hooks with dependencies on hot reloadReactFresh> can hot reload offscreen componentsReactFresh> remounts classes on every editReactFresh> remounts on conversion from class to function and backReactFresh> can update multiple roots independentlyReactCompositeComponent> should warn aboutsetStatein renderReactCompositeComponent> should warn aboutsetStatein getChildContextReactCompositeComponent> this.state should be updated on setState callback inside componentWillMountReactDOMServerIntegration> legacy context › renders with a call to componentWillMount before getChildContext with clean client renderReactDOMServerIntegration> legacy context › renders with a call to componentWillMount before getChildContext with client render on top of good server markupReactDOMServerIntegration> legacy context › renders with a call to componentWillMount before getChildContext with client render on top of bad server markupSimpleEventPlugin> interactive events, in concurrent mode › flushes pending interactive work before extracting event handlerSimpleEventPlugin> interactive events, in concurrent mode › flushes discrete updates in orderReactDOMServerIntegrationUserInteraction> user interaction with controlled inputs › renders a controlled text input with clean client renderReactDOMServerIntegrationUserInteraction> user interaction with controlled inputs › renders a controlled text input with client render on top of good server markupReactDOMServerIntegrationUserInteraction> user interaction with controlled inputs › renders a controlled textarea with clean client renderReactDOMServerIntegrationUserInteraction> user interaction with controlled inputs › renders a controlled textarea with client render on top of good server markupReactDOMServerIntegrationUserInteraction> user interaction with controlled inputs › renders a controlled checkbox with clean client renderReactDOMServerIntegrationUserInteraction> user interaction with controlled inputs › renders a controlled checkbox with client render on top of good server markupReactBrowserEventEmitter> should not invoke newly inserted handlers while bubblingReactDOMServerSelectiveHydration> hydrates at higher pri if sync did not work first timeReactDOMServerSelectiveHydration> hydrates at higher pri for secondary discrete eventsReactES6Class> renders only once when setting state in componentWillMountmixing responders with the heritage event system> should properly flush sync when the event systems are mixed with unstable_flushDiscreteUpdatesmixing responders with the heritage event system> mixing the Input and Press repsonders › is async for non-input eventsReactTypeScriptClass> renders only once when setting state in componentWillMountReactCoffeeScriptClass> renders only once when setting state in componentWillMountReactDOMHooks> should not bail out when an update is scheduled from within an event handler in Concurrent ModeReactIncrementalScheduling> can opt-out of batching using unbatchedUpdatesReactCompositeComponent-state> should support setting stateReactCompositeComponent-state> should treat assigning to this.state inside cWM as a replaceState, with a warningReactDOMComponentTree> finds a controlled instance from node and gets its current fiber props