[Fabric] Fix targetAsInstance dispatchEvent "cannot read property of null" #18156
Conversation
…ull in some cases
|
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 69c4f24:
|
|
Do you need this in the next sync? |
|
Eli thinks it “shouldn’t be possible” for stateNode to be null so I need to do a bit more investigation. I would probably just land the eventual solution as a partial sync if it’s urgent. |
|
fwiw we do null stateNode since recently on unmount. |
|
@gaearon Oh interesting - so the case where it's crashing, it would potentially mean an event has been dispatched to an unmounted component? |
|
@gaearon If it seems to you that this is a reasonable check to make, and that we aren't covering up some earlier data corruption, then I'm fine landing this. |
Possibly.
I don't know the event system enough to tell if that's ever supposed to happen. @trueadm might know. |
|
It's definitely possible with sync events. An event can cause a sync update that causes an update that unmounts a fiber, then the event can propagate to the unmounted fiber. It's rare, but can happen and should probably be guarded against. |
|
Cool. Thanks for the context (cc @TheSavior) |
Summary
In some cases in prod testing, the
targetFiberhere indispatchEventdoes not have a stateNode. If we try to getcanonicaldirectly from stateNode, it will crash. We already check thattargetFiberis non-null before continuing, I think it makes sense to check stateNode as well.It's unclear to me why this is happening in the first place, though (why we'd have a targetFiber but not a stateNode). We may want to investigate and fix that in addition to/instead of this.
Test Plan
yarn testyarn test-prodyarn flow fabricI have an FB-internal prod test as well.