Avoid reactivity bugs in how we track external state #3316
Conversation
robintown
added
X-Blocked
Many of our hooks which attempt to bridge external state from an EventEmitter or EventTarget into React had subtle bugs which could cause them to fail to react to certain updates. The conditions necessary for triggering these bugs are explained by the tests that I've included. In the majority of cases, I don't think we were triggering these bugs in practice. They could've become problems if we refactored our components in certain ways. The one concrete case I'm aware of in which we actually triggered such a bug was the race condition with the useRoomEncryptionSystem shared secret logic (addressed by a1110af). But, particularly with all the weird reactivity issues we're debugging this week, I think we need to eliminate the possibility that any of the bugs in these hooks are the cause of our current headaches.
robintown
force-pushed
the
robin/event-emitter-state
branch
from
f588c85 to
0728bb0
Compare
| const currentState = useTypedEventEmitterState( | ||
| room, | ||
| RoomEvent.CurrentStateUpdated, | ||
| useCallback(() => room.currentState, [room]), | ||
| ); | ||
| return useTypedEventEmitterState( | ||
| currentState, | ||
| RoomStateEvent.Update, | ||
| useCallback(() => setNumUpdates((n) => n + 1), [setNumUpdates]), | ||
| useCallback(() => f(currentState), [f, currentState]), | ||
| ); |
There was a problem hiding this comment.
why id is it not enough, to listen to RoomEvent.CurrentStateUpdated?
Shouldnt that also update the currentState if currentState emits RoomStateUpdate.Update
There was a problem hiding this comment.
No, it's only called when the currentState object is replaced with something else: https://github.com/matrix-org/matrix-js-sdk/blob/44399f601733c8f4daf5a6c891a355cf425192a9/src/models/room.ts#L1319
The way I see it, it's currently impossible to use this correctly in a reactive manner. This chain of methods depends on the current values of:
And I have no idea what events you're supposed to listen to to stay up to date on all three links in the chain. All three of these seem to have cases where matrix-js-sdk can modify them without emitting anything.
|
|
I think on the |
* Avoid reactivity bugs in how we track external state Many of our hooks which attempt to bridge external state from an EventEmitter or EventTarget into React had subtle bugs which could cause them to fail to react to certain updates. The conditions necessary for triggering these bugs are explained by the tests that I've included. In the majority of cases, I don't think we were triggering these bugs in practice. They could've become problems if we refactored our components in certain ways. The one concrete case I'm aware of in which we actually triggered such a bug was the race condition with the useRoomEncryptionSystem shared secret logic (addressed by a1110af). But, particularly with all the weird reactivity issues we're debugging this week, I think we need to eliminate the possibility that any of the bugs in these hooks are the cause of our current headaches. * Reuse useTypedEventEmitterState in useLocalStorage * Fix type error
Many of our hooks which attempt to bridge external state from an EventEmitter or EventTarget into React had subtle bugs which could cause them to fail to react to certain updates. The conditions necessary for triggering these bugs are explained by the tests that I've included.
In the majority of cases, I don't think we were triggering these bugs in practice. They could've become problems if we refactored our components in certain ways. The one concrete case I'm aware of in which we actually triggered such a bug was the race condition with the
useRoomEncryptionSystemshared secret logic (addressed by a1110af).But, particularly with all the weird reactivity issues we're debugging this week, I think we need to eliminate the possibility that any of the bugs in these hooks are the cause of our current headaches.