[React Native] Fabric get current event priority #21553
Conversation
|
Comparing: 5aa0c56...f170db8 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
sammy-SC
force-pushed
the
rn-fabric-getCurrentEventPriority
branch
from
331f702 to
761c937
Compare
sammy-SC
changed the title
Fix flow errors
sammy-SC
force-pushed
the
rn-fabric-getCurrentEventPriority
branch
from
23a4e33 to
61d10f9
Compare
rickhanlonii
changed the title
|
@gaearon we're just handing discrete for now per @sebmarkbage, but we can expand as it makes sense. |
| @@ -343,6 +349,18 @@ export function shouldSetTextContent(type: string, props: Props): boolean { | |||
| } | |||
|
|
|||
| export function getCurrentEventPriority(): * { | |||
| const currentEventPriority = fabricGetCurrentEventPriority(); | |||
There was a problem hiding this comment.
What if fabricGetCurrentEventPriority is not defined or nativeFabricUIManager doesn't exist?
There was a problem hiding this comment.
if fabricGetCurrentEventPriority is not defined, Fabric returns undefined.
We guarantee existence of nativeFabricUIManager. If it doesn't exist, something is very wrong.
| @@ -343,6 +349,18 @@ export function shouldSetTextContent(type: string, props: Props): boolean { | |||
| } | |||
|
|
|||
| export function getCurrentEventPriority(): * { | |||
| const currentEventPriority = fabricGetCurrentEventPriority(); | |||
|
|
|||
| if (currentEventPriority !== undefined) { | |||
There was a problem hiding this comment.
So null would be the Default priority? That's how it ends up working in DOM, but just making sure that's intended.
There was a problem hiding this comment.
I added this condition here for safety. If Fabric returns undefined, which it never should, we fall back to the old behaviour.
| const currentEventPriority = fabricGetCurrentEventPriority(); | ||
|
|
||
| if (currentEventPriority !== undefined) { | ||
| switch (currentEventPriority) { |
There was a problem hiding this comment.
One thing we discussed is that instead of native proving priority, they could send provide the name of the event here, and in this switch we would select the priority based on event name (like DOM does).
The advantage of this approach is that we can update this map in JavaScript without needing a native app change, and the source of truth for "event name to event priority" lives inside React (leaving us free to change the model cheaply). The disadvantage is that we'd need to maintain the map here for all React Native platforms, but as soon as we change the model we would need to maintain an event priority to new model priority map anyway.
@sebmarkbage @acdlite what do you think about those two approaches?
There was a problem hiding this comment.
re: sending event name of event
This doesn't work well with event priority deduction that we have implemented in Fabric.
I do have a somewhat hybrid system in mind that would combine event priority deduction with event priority map in JavaScript. I didn't want to overcomplicate the initial implementation and learn from this before making the next step.
The APIs are still marked as "unstable" to make it clear we are still experimenting with it.
Summary: Changelog: [internal] Use getter function instead of accessing ivar. This makes for clearer APIs. React part is implemented in facebook/react#21553 Reviewed By: JoshuaGross Differential Revision: D28675372 fbshipit-source-id: cf99f8482067bfc0fd57d39fda9656abd665bb69
Summary: This sync includes the following changes: - **[c96b78e0e](facebook/react@c96b78e0e )**: Add concurrentRoot property to ReactNativeTypes ([#21648](facebook/react#21648)) //<Samuel Susla>// - **[1a3f1afbd](facebook/react@1a3f1afbd )**: [React Native] Fabric get current event priority ([#21553](facebook/react#21553)) //<Samuel Susla>// - **[48a11a3ef](facebook/react@48a11a3ef )**: Update next React version ([#21647](facebook/react#21647)) //<Andrew Clark>// - **[5aa0c5671](facebook/react@5aa0c5671 )**: Fix Issue with Undefined Lazy Imports By Refactoring Lazy Initialization Order ([#21642](facebook/react#21642)) //<Sebastian Markbåge>// Changelog: [General][Changed] - React Native sync for revisions 0eea577...c96b78e jest_e2e[run_all_tests] Reviewed By: bvaughn Differential Revision: D29029542 fbshipit-source-id: 9f2e19b4714b5697b5b846f2db8eb28c25420932
* Call into Fabric to get current event priority Fix flow errors * Prettier * Better handle null and undefined cases * Remove optional chaining and use ?? operator * prettier-all * Use conditional ternary operator * prettier
Summary
Replace default implementation of
getCurrentEventPrioritywith a call to Fabric to check current event's priority.