[Fabric] Extract Fabric event handlers from canonical props #13024
Conversation
Details of bundled changes.Comparing: 1594409...cb76d46 react-native-renderer
Generated by 🚫 dangerJS |
|
What’s the long term plan? I assume updating in render phase is not it? |
|
I wasn’t actually sure what semantics made the most sense. However I spent tonight thinking about it and I think the only way imperative callbacks make sense is on a single timeline. Even if they’re scheduled on different threads and different default priorities. That means that if we have two async events like A and B that gets dispatched to the background thread. But C is sync on the main thread, takes the lock, it can execute in between them. So the timeline looks like A -> C -> B. Then it is as if B happened after C. Which is weird because C might have changed the responder or something which would’ve made B impossible to fire. But as far as JS is concerned, the sequence was A -> C -> B. So if C updated some state, that should be available to the callback fired in B. That seems to indicate that we want “last committed props” semantics. One way to achieve that is adding a commit phase hook which is how we do events in the mutable mode too. Assuming A and B are serial events, they should always be guaranteed to fully flush before invoking the next serial event. So B always sees the changes of A, but the interesting scenario is when some sync work happens between the event happening on the main thread and it actually firing on the background thread. Another possible semantic is that in that scenario we force the event to happen, eg on the main thread before invoking the sync event. But don’t actually render the result. That way there is never a conflict in imperative code. |
We need a different "component tree" thingy for Fabric. A lot of this doesn't really make much sense in a persistent world but currently we can't dispatch events to memoizedProps on a Fiber since they're pooled. Also, it's unclear what the semantics should be when we dispatch an event that happened when the old props were in effect but now we have new props already. This implementation tries to use the last committed props but also fails at that because we don't have a commit hook in the persistent mode. However, at least it doesn't crash when dispatching. :)
| @@ -293,6 +293,8 @@ export function prepareUpdate( | |||
| ); | |||
| // TODO: If the event handlers have changed, we need to update the current props | |||
| // in the commit phase but there is no host config hook to do it yet. | |||
| // So instead we hack it by updating it in the render phase. | |||
| instance.canonical.currentProps = newProps; | |||
There was a problem hiding this comment.
So this is sufficient to fix the crashes? How important is the follow-up PR to add a commit phase hook?
There was a problem hiding this comment.
It doesn't matter for sync mode but will be important eventually.
I'm considering using the memoizedProps solution to avoid having to add a commit phase in JS. Currently the commit phase is entire in native.
We need a different "component tree" thingy for Fabric.
A lot of this doesn't really make much sense in a persistent world but
currently we can't dispatch events to memoizedProps on a Fiber since
they're pooled. Also, it's unclear what the semantics should be when we
dispatch an event that happened when the old props were in effect but now
we have new props already.
This implementation tries to use the last committed props but also fails
at that because we don't have a commit hook in the persistent mode.
However, at least it doesn't crash when dispatching. :)