Add end-of-frame scheduling for default events #24594
Closed
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
rickhanlonii
commented
May 22, 2022
acdlite
reviewed
May 23, 2022
It's the same as any other setState. Shouldn't need to do anything special.
Don't need to for our tests unless we're testing the behavior of React DOM specifically. In the open source build, people will either use
Nah I can't think of any reason why it should
Sounds right to me, that's where
The only place in the reconciler where there should be a fork in behavior is in
The main ones I can think of are
|
|
Thanks for all the answers @acdlite!
My bad, I must have misheard that we wanted a separate lane. I'll switch this to a FiberRoot flag 👍
When |
Yeah I think so |
rickhanlonii
force-pushed
the
rh-unknown-lane
branch
from
3fd61cb
to
1ab4eaa
Compare
rickhanlonii
changed the title
|
Comparing: 8186b19...7d03b23 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
force-pushed
the
rh-unknown-lane
branch
2 times, most recently
from
f171cfb
to
10483f6
Compare
rickhanlonii
changed the title
|
I believe this is ready for another review 🤝 |
acdlite
reviewed
May 31, 2022
| newCallbackPriority === DefaultLane && | ||
| existingFrameAlignedNode == null && | ||
| typeof window !== 'undefined' && | ||
| typeof window.event === 'undefined' |
There was a problem hiding this comment.
This logic needs to move to the Host Config because it's DOM-specific.
acdlite
reviewed
May 31, 2022
acdlite
reviewed
May 31, 2022
acdlite
reviewed
May 31, 2022
acdlite
reviewed
May 31, 2022
acdlite
reviewed
May 31, 2022
| }); | ||
| expect(Scheduler).toHaveYielded(['Count: 0']); | ||
|
|
||
| window.event = 'test'; |
There was a problem hiding this comment.
Nit: I would use an actual DOM event here. Like with dispatchEvent.
acdlite
reviewed
May 31, 2022
rickhanlonii
force-pushed
the
rh-unknown-lane
branch
2 times, most recently
from
1790519
to
3d41b27
Compare
gaearon
reviewed
Jun 8, 2022
packages/react-reconciler/README.md
Outdated
| @@ -210,6 +210,17 @@ Set this to true to indicate that your renderer supports `scheduleMicrotask`. We | |||
|
|
|||
| Optional. You can proxy this to `queueMicrotask` or its equivalent in your environment. | |||
|
|
|||
| #### `supportsFrameEndTask` | |||
rickhanlonii
force-pushed
the
rh-unknown-lane
branch
2 times, most recently
from
82daf47
to
0d53dba
Compare
acdlite
reviewed
Jun 9, 2022
| newCallbackPriority === DefaultLane && | ||
| root.hasUnknownUpdates | ||
| ) { | ||
| // Do nothing, we need to cancel the existing default task and schedule a rAF. |
There was a problem hiding this comment.
This seems wrong because what if a rAF were already scheduled? It would cancel it and schedule a new one, which might cause us to get shifted into a later frame
There was a problem hiding this comment.
Ah yeah, good point. I'll update.
There was a problem hiding this comment.
@acdlite so when should we cancel the raf and scheduler task in this case? If we don't cancel here, we'll orphan existingCallbackNode without canceling the task. One option is to cancel it only if the newCallbackPriority is not default. That could push things to the next frame if something higher priority comes in like a click, but may be desirable so that the sync microtask fires first anyway?
acdlite
reviewed
Jun 9, 2022
rickhanlonii
force-pushed
the
rh-unknown-lane
branch
from
0d53dba
to
f346a1f
Compare
acdlite
reviewed
Jun 9, 2022
| // requestAnimationFrame | ||
| // ------------------- | ||
| export const supportsFrameEndTask = true; | ||
| export function scheduleFrameEndTask(task) { |
There was a problem hiding this comment.
I think "frame aligned" is better than "frame end", it doesn't matters where it fires in the refresh cycle as long as it happens before the next paint
rickhanlonii
force-pushed
the
rh-unknown-lane
branch
5 times, most recently
from
c54e365
to
0a0565e
Compare
rickhanlonii
force-pushed
the
rh-unknown-lane
branch
from
0a0565e
to
e012516
Compare
1 task
|
Closing because I think the unified sync lane stuff replaces this. |
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.
Overview
This PR schedules a second task for default updates at the end of the frame when we cannot determine the priority of the originating event. These updates include things like
IntersectionObserver,ResizeObserver,setTimeout, or any other event wherewindow.eventis undefined.These updates are flushed inside
requestAnimationFramein addition to the task, so that updates scheduled inside of things likeResizeObservershave an opportunity to flush before the next repaint. We'll use whichever task fires first.TODO Incomplete
supportsAnimationFrame,scheduleAnimationFrame, andcancelAnimationFrameTODO Questions
root.renderorhydrateRoot?requestAnimationFrame(it doesn't flush with timers in jsdom)?internalAct.ReactNoopRendererinclude arAFmock?scheduleAnimationFramelikeschedulePostTask