-
Notifications
You must be signed in to change notification settings - Fork 47.5k
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
Experiment: Infer the current event priority from the native event #20748
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,7 @@ import { | |
enableDoubleInvokingEffects, | ||
skipUnmountedBoundaries, | ||
enableTransitionEntanglement, | ||
enableNativeEventPriorityInference, | ||
} from 'shared/ReactFeatureFlags'; | ||
import ReactSharedInternals from 'shared/ReactSharedInternals'; | ||
import invariant from 'shared/invariant'; | ||
|
@@ -94,6 +95,7 @@ import { | |
afterActiveInstanceBlur, | ||
clearContainer, | ||
scheduleMicrotask, | ||
getCurrentEventPriority, | ||
} from './ReactFiberHostConfig'; | ||
|
||
import { | ||
|
@@ -461,11 +463,23 @@ export function requestUpdateLane(fiber: Fiber): Lane { | |
const currentLanePriority = getCurrentUpdateLanePriority(); | ||
lane = findUpdateLane(currentLanePriority, currentEventWipLanes); | ||
} else { | ||
const schedulerLanePriority = schedulerPriorityToLanePriority( | ||
schedulerPriority, | ||
); | ||
|
||
lane = findUpdateLane(schedulerLanePriority, currentEventWipLanes); | ||
if (enableNativeEventPriorityInference) { | ||
const eventLanePriority = getCurrentEventPriority(); | ||
if (eventLanePriority === DefaultLanePriority) { | ||
// TODO: move this case into the ReactDOM host config. | ||
const schedulerLanePriority = schedulerPriorityToLanePriority( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be in a follow-up, but let's move this Scheduler consultation to React DOM. We only need to check it for message events, since that's the event Scheduler uses internally. Conceptually, we're asking the renderer "what is the priority of the current platform event?" Scheduler isn't part of the The Platform but it's standing in for And React Native will probably do something completely different, so makes sense to move this logic to the renderer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently, this can return Idle or NoLane priorities. Are these also valid? E.g. would we expose the associated constants and consider them fair game to return from third party renderers? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, would it be ok for ReactDOM to reach into |
||
schedulerPriority, | ||
); | ||
lane = findUpdateLane(schedulerLanePriority, currentEventWipLanes); | ||
} else { | ||
lane = findUpdateLane(eventLanePriority, currentEventWipLanes); | ||
} | ||
} else { | ||
const schedulerLanePriority = schedulerPriorityToLanePriority( | ||
schedulerPriority, | ||
); | ||
lane = findUpdateLane(schedulerLanePriority, currentEventWipLanes); | ||
} | ||
} | ||
|
||
return lane; | ||
|
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.
To address @sebmarkbage's concern about leaking internals, can we move this to a reconciler export? Something like:
and
Then later we can change that export to be whatever we want, even if it requires us casting to
any
(though they're all justnumber
anyway).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.
So for example, a future PR would change ReactFiberReconciler to look like:
without needing to change any of the renderers
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.
For default only?
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.
All of them
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.
Well all of the ones that are needed by the event system, I mean
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't do this after all, because it creates a circular import chain (Reconciler -> HostConfig -> Reconciler). There are probably smarter ways to break it off but for now I'm keeping the old structure (direct imports) but exporting some constants just for third-party renderers.