Investigate using a single store for Loop Rooms

RESOLVED FIXED in Firefox 35

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: NiKo, Unassigned)

Tracking

unspecified
mozilla36
Points:
---
Bug Flags:
in-qa-testsuite -
in-testsuite +
in-moztrap -
qe-verify -

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Basic proof of concept of the idea.
Attachment #8516686 - Flags: feedback?(standard8)
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
Attachment #8516686 - Flags: feedback?(standard8) → feedback+
Addressed comments, added more tests. This is now ready for review I think.
Attachment #8516686 - Attachment is obsolete: true
Attachment #8516805 - Flags: review?(standard8)
Updated patch to remove all references to local room ids (now obsolete). Added more tests.
Attachment #8516805 - Attachment is obsolete: true
Attachment #8516805 - Flags: review?(standard8)
Attachment #8516958 - Flags: review?(standard8)
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
Attachment #8516958 - Flags: review?(standard8) → review+
https://hg.mozilla.org/integration/fx-team/rev/9a1524fcdbb0
Assignee: nobody → nperriault
Iteration: --- → 36.2
Target Milestone: --- → mozilla36
https://hg.mozilla.org/mozilla-central/rev/9a1524fcdbb0
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Flags: qe-verify-
Flags: in-qa-testsuite-
Flags: in-moztrap-
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
Attachment #8516958 - Flags: approval-mozilla-aurora?
Attachment #8516958 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.