1079202 - Loop Flux actions should dispatch events directly; Components shouldn't know about the dispatcher

Status: RESOLVED INCOMPLETE

(Hello (Loop) :: Client, defect, P4)

Description

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

TL;DR we should <Comp store={store} actions={actions}/> instead of <Comp store={store} dispatcher={dispatcher}/>

Right now the Flux implementation in Loop assume you're passing the dispatcher to every component needing to perform an action; usually we should be passing them an actions object, responsible of dispatching stuff.

That means, instead of dispatching right from the component, we could just call action methods:

// before
handleClick: function() {
  this.props.dispatcher.dispatch(new sharedActions.MyAction({my: "payload"}));
}

// after
handleClick: function() {
  this.props.actions.myAction({my: "payload"}));
}

This is shorter, but most importantly hides the dispatcher — which is an implementation detail — from the component.

Comment 1

[Pablo Almenar]

I'm working on getting this done, please assign it to me.

Comment 2

[Mark Banner (:standard8)]

Pablo: Thanks for offering to take this on. This is a wide ranging change, so please be sure to post some small incomplete versions early on for feedback. I don't want you doing a lot of change and then discovering the API isn't exactly what you want.

You can always prove out the early versions work by adjusting the unit tests as you go.

If there's a sensible way to maintain the old style for some actions and the new whilst we transition across then it might help to avoid bitrot (its ok to land multiple patches).


Additionally, I think that we can do slightly better than comment 0 - passing in actions/dispatchers to components doesn't really help us. I'd like to see something more like the new interfaces we're introducing in attachment 8680660 [details] [diff] [review] - something like:

loop.actions.MyAction({my: "payload"});

Comment 3

[Pablo Almenar]

Attachment: First fix attempt

Thanks a lot Mark! I'm submitting an incomplete attempt as you said, so you can tell me what you think.

I tried to do it in a way that the old style still works, and the usage would instead be 'new loop.actions.MyAction({my: "payload"}).dispatch();'. Is that okay?

Comment 4

[Pablo Almenar]

(In reply to Pablo Almenar from comment #3)
> Created attachment 8684410 [details] [diff] [review]
> First fix attempt
> 
> Thanks a lot Mark! I'm submitting an incomplete attempt as you said, so you
> can tell me what you think.
> 
> I tried to do it in a way that the old style still works, and the usage
> would instead be 'new loop.actions.MyAction({my: "payload"}).dispatch();'.
> Is that okay?

Note: What I did there was to change the Action class so that it works as requested while allowing the old style, and change the way that one component (DesktopRoomConversationView) uses it so you can see what it looks like.

I made a typo in the previous patch btw, the usage should be something like this instead:

let action = new loop.actions.MyAction({my: "payload"});
action.dispatch();

Comment 5

[Mark Banner (:standard8)]

Hi Pablo, sorry for the delay in getting back to you. I'm currently trying to discuss this with a couple of other people, and haven't had time. I'll try and get a discussion with them later today.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8684410 [details] [diff] [review]
First fix attempt

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

Hi Pablo, I've finally had the discussions I was trying to have. Sorry for the delay.

Having looked at this, I was wrong to suggest the backwards compatibility in that way. We'd prefer to go straight to the API with the simple `loop.actions.SomeAction({"my": payload});`

Like before, I'd suggest playing around with the new api in a small patch and getting that up for feedback again. I'm happy to work through this with you and should be able to be a bit more responsive now.

Comment 7

[Pablo Almenar]

Attachment: New API

Here's an attempt of implementing the new API as you said. 

Only for 'AppControllerView' for now, since as you suggested I'm only playing around for now.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8690403 [details] [diff] [review]
New API

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

This is a good start and I like the new basic API. There's a few comments about the code, but its time to move forward!

::: browser/components/loop/content/shared/js/actions.js
@@ +6,5 @@
>  loop.shared = loop.shared || {};
>  loop.shared.actions = (function() {
>    "use strict";
>  
> +  var dispatcher = new loop.Dispatcher();

I think we'll need to change how this is constructed, as the individual stores need to be able to register with it.

Maybe rather than constructing it, we should change the dispatcher to just be functions that are accessible, e.g. loop.dispatcher.dispatch(), loop.dispatcher.register() etc.

@@ +19,5 @@
>    function Action(name, schema, values) {
> +    let action = {
> +      name: name
> +    };
> +    let validatedData = new loop.validate.Validator(schema || {})

Unfortunately we can't use let in these files - they are shared with our web pages, and we have to have them compatible with older browsers.

@@ +27,2 @@
>  
> +    Action.dispatch(action);

I think I marginally prefer dispatcher.dispatch(this) with the properties being set on the object. Then we're actually passing around the type of action, rather than just an object.

Comment 9

[Pablo Almenar]

(In reply to Mark Banner (:standard8) from comment #8)
> Comment on attachment 8690403 [details] [diff] [review]
> New API
> 
> Review of attachment 8690403 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a good start and I like the new basic API. There's a few comments
> about the code, but its time to move forward!
> 
> ::: browser/components/loop/content/shared/js/actions.js
> @@ +6,5 @@
> >  loop.shared = loop.shared || {};
> >  loop.shared.actions = (function() {
> >    "use strict";
> >  
> > +  var dispatcher = new loop.Dispatcher();
> 
> I think we'll need to change how this is constructed, as the individual
> stores need to be able to register with it.
> 
> Maybe rather than constructing it, we should change the dispatcher to just
> be functions that are accessible, e.g. loop.dispatcher.dispatch(),
> loop.dispatcher.register() etc.
> 
> @@ +19,5 @@
> >    function Action(name, schema, values) {
> > +    let action = {
> > +      name: name
> > +    };
> > +    let validatedData = new loop.validate.Validator(schema || {})
> 
> Unfortunately we can't use let in these files - they are shared with our web
> pages, and we have to have them compatible with older browsers.
> 
> @@ +27,2 @@
> >  
> > +    Action.dispatch(action);
> 
> I think I marginally prefer dispatcher.dispatch(this) with the properties
> being set on the object. Then we're actually passing around the type of
> action, rather than just an object.

Thanks a lot for your feedback!

You're right about my patch being completely wrong due to the dispatcher instances. I took a closer look at the code and it seems that this may be a bigger overhaul than it may seem, although the result will be a lot better readability and simplicity of the system and I'm pretty sure I can do it.

Right now, if I got it right, every window (which means the panel, the webapp and each conversation) has an instance of the Dispatcher that is common to all its components and stores. If I set a globally available dispatcher instead, the windows would interfere, so I think I got to set up a way to distinguish them in event handling (so eg muting one conversation's sound doesn't mute all) and maybe make Stores global instances too.

Any tips before I get to it?

Comment 10

[Mark Banner (:standard8)]

(In reply to Pablo Almenar from comment #9)
> Right now, if I got it right, every window (which means the panel, the
> webapp and each conversation) has an instance of the Dispatcher that is
> common to all its components and stores. If I set a globally available
> dispatcher instead, the windows would interfere, so I think I got to set up
> a way to distinguish them in event handling (so eg muting one conversation's
> sound doesn't mute all) and maybe make Stores global instances too.

Each window is sandboxed - none of them can interact directly. So one dispatcher per window is fine. You could define it globally or drop the "new", and just have global functions on the `loop` object, see the recently added loopapi-client.js for example.

Comment 11

[Pablo Almenar]

Attachment: globalDispatcher.patch

Hey. Do you think this would do the trick?

I modified Actions and Dispatcher to allow for the new API I told you, only changing the usage would be left. Neither stores nor components would need to be passed a dispatcher anymore.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8694296 [details] [diff] [review]
globalDispatcher.patch

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

Yes, this looks good. I like the simplifications here!

Also, just to note we're likely moving to github at the start of next week. I'll keep you posted.

::: browser/extensions/loop/content/shared/js/dispatcher.js
@@ +62,5 @@
> +   *
> +   * @param {Object} store The store object to register
> +   * @param {Array} eventTypes An array of action names
> +   */
> +  loop.registerStore = function registerStore(store, eventTypes) {

Maybe this should become loop.registerStoreActions as a slightly clearer name?

Comment 13

[Pablo Almenar]

(In reply to Mark Banner (:standard8) from comment #12)
> Comment on attachment 8694296 [details] [diff] [review]
> globalDispatcher.patch
> 
> Review of attachment 8694296 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes, this looks good. I like the simplifications here!
> 
> Also, just to note we're likely moving to github at the start of next week.
> I'll keep you posted.
> 
> ::: browser/extensions/loop/content/shared/js/dispatcher.js
> @@ +62,5 @@
> > +   *
> > +   * @param {Object} store The store object to register
> > +   * @param {Array} eventTypes An array of action names
> > +   */
> > +  loop.registerStore = function registerStore(store, eventTypes) {
> 
> Maybe this should become loop.registerStoreActions as a slightly clearer
> name?

Hey man. I finished this change and it's working. There's a whole lot of tests to adjust though and just that grunt work is taking me some time, but I will deliver soon, even though I'm gonna have a few busy days ahead. If by the time it's finished you guys have switched this over to GitHub, I can just do a pull request, right? My GitHub email is the same as in here.

Comment 14

[Mark Banner (:standard8)]

(In reply to Pablo Almenar from comment #13)
> Hey man. I finished this change and it's working. There's a whole lot of
> tests to adjust though and just that grunt work is taking me some time, but
> I will deliver soon, even though I'm gonna have a few busy days ahead. If by
> the time it's finished you guys have switched this over to GitHub, I can
> just do a pull request, right? My GitHub email is the same as in here.

Excellent great. We've now pretty much moved to github, its a bit rough at the moment, but make install, make build & make runserver should work. We're working on the documentation as well.

The repo is here:

https://github.com/mozilla/loop

Comment 15

[Pablo Almenar]

Attachment: globalDispatcher.patch

Hey, this is the working version, I'm asking for feedback so you can tell me what you think before taking in to GitHub.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8700335 [details] [diff] [review]
globalDispatcher.patch

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

This is looking great so far. Though I think we can improve things for the tests - I don't really like the way we have to repeat how each action is constructed. Mike suggested that we do something like this against your patch:

diff --git a/browser/extensions/loop/content/shared/js/actions.js b/browser/extensions/loop/content/shared/js/actions.js
index 80915c6..f4c15a9 100644
--- a/browser/extensions/loop/content/shared/js/actions.js
+++ b/browser/extensions/loop/content/shared/js/actions.js
@@ -29,6 +29,9 @@ loop.shared.actions = (function() {
       var action = new Action(name, schema, values);
       loop.dispatchAction(action);
     };
+    dispatch.get = function(values) {
+      return new Action(name, schema, values);
+    }
     return dispatch;
   }
 
The tests can use the get function like this:

store.roomFailure(sharedActions.RoomFailure.get({
  error: fakeError,
  failedJoinRequest: false
}));

Although we have to remember to put the .get in there, I think its going to be a lot easier to maintain like that.

The only other thing I'm really concerned about are the changes in activeRoomStore#fetchServerData. They would currently mean that the actions would be dispatched when they are received from either the server or the browser, but the way it is designed, is that they need to be dispatched at the same time to avoid display changing rapidly. With this new .get function, I think we can revert just those changes (fetchServerData, _getRoomDataForStandalone, _promiseDetectUserAgentHandles), use the .get function for the actions, and then still dispatch everything once the promise completes.

Definitely great work so far, I think we're not too far off now.

::: browser/extensions/loop/content/panels/js/conversationAppStore.js
@@ +112,5 @@
>        }
>  
>        this.setStoreState({ windowType: windowData.type });
>  
> +      loop.shared.actions.SetupWindowData(_.extend({

Might be nice to switch to sharedActions in this file.

Comment 17

[Pablo Almenar]

Attachment: globalDispatcher.patch

Thank you a lot for your feedback!

I just applied it in this new patch.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 8700794 [details] [diff] [review]
globalDispatcher.patch

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

I've only taken a brief look through the files but this looks a lot better - I like the new .get API. That works nicely.

Comment 19

[Mark Banner (:standard8)]

Support for Hello/Loop has been discontinued.

https://support.mozilla.org/kb/hello-status

Hence closing the old bugs. Thank you for your support.