1093620 - Investigate using a single store for Loop Rooms

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Separating stores for rooms makes sharing code logic harder than necessary; we should investigate using the Cursor pattern (a cursor is basically a piece of the application state that knows how to update itself) to allow using a single store (and substores) to ease subscribing to substate changes, while substores could implement their very own logic.

Comment 1

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Using a substore for Rooms (PoC).

Basic proof of concept of the idea.

Comment 2

[Mark Banner (:standard8) (afk until 26 Oct)]

Comment on attachment 8516686 [details] [diff] [review]
Using a substore for Rooms (PoC).

Review of attachment 8516686 [details] [diff] [review]:
-----------------------------------------------------------------

Generally looking good, I think its worth moving forward into code, as it seems a better way of doing things and we'll only know for sure once we start using it.

::: browser/components/loop/content/js/roomViews.jsx
@@ +25,4 @@
>      },
>  
>      componentWillMount: function() {
> +      this.listenTo(this.props.roomStore, "change:localStore",

This needs to be consistent with the key name for the event in roomStore.

@@ +35,5 @@
>       *
>       * @private
>       */
> +    _onActiveRoomStoreChanged: function() {
> +      this.setState(this.props.roomStore.getStoreState().localStore);

localStore -> proper name.

::: browser/components/loop/content/shared/js/roomStore.js
@@ +68,5 @@
>      }
>      this._mozLoop = options.mozLoop;
>  
> +    if (!options.activeRoomStore) {
> +      options.activeRoomStore = new loop.store.ActiveRoomStore({

As discussed, this should be an optional parameter.

@@ +114,5 @@
>        error: null,
>        pendingCreation: false,
>        pendingInitialRetrieval: false,
> +      rooms: [],
> +      localRoom: {}

Need to change localRoom -> activeRoom throughout the file.

@@ +163,5 @@
>        this._mozLoop.rooms.on("delete", this._onRoomRemoved.bind(this));
>      },
>  
>      /**
> +     * Updates local room store state.

local -> active

Comment 3

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Using a single root store for Rooms.

Addressed comments, added more tests. This is now ready for review I think.

Comment 4

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Using a single root store for Rooms.

Updated patch to remove all references to local room ids (now obsolete). Added more tests.

Comment 5

[Mark Banner (:standard8) (afk until 26 Oct)]

Comment on attachment 8516958 [details] [diff] [review]
Using a single root store for Rooms.

Review of attachment 8516958 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r=Standard8

Comment 6

[Mark Banner (:standard8) (afk until 26 Oct)]

https://hg.mozilla.org/integration/fx-team/rev/9a1524fcdbb0

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a1524fcdbb0

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5b7c6ca135eb

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8516958 [details] [diff] [review]
Using a single root store for Rooms.

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only