-
-
Notifications
You must be signed in to change notification settings - Fork 833
Implement a store for RoomView #921
Implement a store for RoomView #921
Conversation
This allows for a truely flux-y way of storing the currently viewed room, making some callbacks (like onRoomIdResolved) redundant and making sure that the currently viewed room (ID) is only stored in one place as opposed to the previous many places. This was required for the `join_room` action which can be dispatched to join the currently viewed room. Another change was to introduce `LifeCycleStore` which is a start at encorporating state related to the lifecycle of the app into a flux store. Currently it only contains an action which will be dispatched when the sync state has become PREPARED. This was necessary to do a deferred dispatch of `join_room` following the registration of a PWLU (PassWord-Less User). The following actions are introduced: - RoomViewStore: - `view_room`: dispatch to change the currently viewed room ID - `join_room`: dispatch to join the currently viewed room - LifecycleStore: - `do_after_sync_prepared`: dispatch to store an action which will be dispatched when `sync_state` is dispatched with `state = 'PREPARED'` - MatrixChat: - `sync_state`: dispatched when the sync state changes. Ideally there'd be a SyncStateStore that emitted an `update` upon receiving this, but for now the `LifecycleStore` will listen for `sync_state` directly.
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.
In general this looks OK. It's nice to have the functional bits split out of roomview. Have you tested:
- navigating to a room alias via the URL bar (does the right alias get to the room preview bar?)
- Clicking a room from the room directory (name + room icon should be displayed in room preview bar)
onRegistered={this.props.onRegistered} | ||
eventId={this.props.initialEventId} | ||
thirdPartyInvite={this.props.thirdPartyInvite} | ||
oobData={this.props.roomOobData} | ||
highlightedEventId={this.props.highlightedEventId} | ||
eventPixelOffset={this.props.initialEventPixelOffset} | ||
key={this.props.currentRoomAlias || this.props.currentRoomId} | ||
key={this.props.currentRoomId} |
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.
won't this be null sometimes?
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.
Yep. As there's only one RoomView, I'll just give it a default key of "roomview".
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.
ok - is this going to break the assumption that a fresh roomview will always be created for a new room if you navigate from one alias to the other (which I think is when the room id is null)?
src/stores/LifecycleStore.js
Outdated
if (payload.state !== 'PREPARED') { | ||
break; | ||
} | ||
console.warn(this._state); |
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.
Why is this a warn?
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.
oops
@@ -891,6 +895,7 @@ module.exports = React.createClass({ | |||
}); | |||
|
|||
cli.on('sync', function(state, prevState) { | |||
dis.dispatch({action: 'sync_state', prevState, state}); |
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.
I don't really get why it's MatrixChat's job to relay this between the client and the store: if the store cares, shouldn't it just subscribe?
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.
The good thing about flux.Store
is that it enforces the rule that everything that calls __emitChange
(which is done after _setState
) must be done during a dispatch. The js-sdk is not a dispatcher so on.('sync', cb)
doesn't count and cb
cannot cause the store to update.
So the store can't subscribe. As per my commit message, ideally there'd be a sync state store which registered as a listener and dispatched to itself a sync_state
which would then propagate the actual sync state.
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.
Eugh, sounds like the general impedance mismatch of the two different dispatch mechanisms, but also sounds like this is the correct way to get around it. A comment might not be a bad idea so you don't have to have this conversation with the next 8 people who read that line.
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.
Good idea, I've added a comment.
otherwise lgtm |
I have tested that functionality btw, forgot to mention. |
This allows for a truely flux-y way of storing the currently viewed room, making some callbacks (like onRoomIdResolved) redundant and making sure that the currently viewed room (ID) is only stored in one place as opposed to the previous many places.
This was required for the
join_room
action which can be dispatched to join the currently viewed room. Fixes element-hq/element-web#3986Another change was to introduce
LifeCycleStore
which is a start at encorporating state related to the lifecycle of the app into a flux store. Currently it only contains an action which will be dispatched when the sync state has become PREPARED. This was necessary to do a deferred dispatch ofjoin_room
following the registration of a PWLU (PassWord-Less User).The following actions are introduced:
view_room
: dispatch to change the currently viewed room IDjoin_room
: dispatch to join the currently viewed roomdo_after_sync_prepared
: dispatch to store an action which will be dispatched whensync_state
is dispatched withstate = 'PREPARED'
sync_state
: dispatched when the sync state changes. Ideally there'd be a SyncStateStore that emitted anupdate
upon receiving this, but for now theLifecycleStore
will listen forsync_state
directly.