1217935 - Make redux reducer events fire at the right time

Status: RESOLVED INVALID

(DevTools :: Debugger, defect)

Description

[James Long (:jlongster)]

We have the concept of emitting events from a redux reducer only for the purpose of integrating with older event-driven UIs. You don't need this at all when using React.

The events need to fire at a specific point in time, so we can't use the normal event-emitter that calls listeners immediately when events are fired. It's not really a big deal, it's only a few lines of code to implement the emitter.

The comments in the patch explain but I'll explain it again here. Here's the workflow with redux:

action fired
 |
 v
reducers handle action, generate new state
 |
 v
UI renders the new state

We've augmented reducers with a 3rd param, `emit`, to emit events for specific state changes to work with older event-based UIs. But we cannot run them immediately, because the UI will run before the reducers have finished, and the state in the store will be stale.

action fired
 |
 v
reducers start handling here ----
 reducers handle action, generate 
 new state                         -----> event A fired -> UI renders piece of state
reducers finish here        -----
 |
 v
UI renders the new state

The above workflow is wrong, the UI will run in the middle of a reducer update. We need to enqueue the events and fire them after the reducers have finished and the store has the new state.

action fired
 |
 v
reducers handle action, generate new state
 |
 v
enqueued events are fired and listeners are called (UI renders the new state)

Note that this entire process is still synchronous, we just wait to fire the events.

Comment 1

[James Long (:jlongster)]

Attachment: 1217935.patch

Jordan I don't think are using this because you are using React, but if you are the API has changed.

Comment 2

[James Long (:jlongster)]

Attachment: 1217935.patch

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8678258 [details] [diff] [review]
1217935.patch

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

::: devtools/client/shared/redux/emitter.js
@@ +56,5 @@
> +    off: (name, cb) => {
> +      listeners[name] = listeners[name].filter(listener => listener !== cb);
> +    },
> +
> +    emit: (name, payload) => {

I would leave normal `emit` with its normal sync/immediate behavior, but make a new method `emitOnDoneState` or `emitOnNextTick` or something so it's clear what's happening. Otherwise, looking at `emit`, I'd assume it's fired just like all of our other uses of `emit`. How does that sound?

Comment 4

[James Long (:jlongster)]

WFM, I wouldn't want to type that in the reducers to emit every event, as it's already pretty verbose to have those in there. We pass the function in though so it's just a generic function. Naming this code more clearly would be good though. I can also rename `makeEmitter` to clarify that it's different.

Comment 5

[James Long (:jlongster)]

I just remembered that I used to actually use `emitChange` in the reducers, not `emit`. If you thing that also clarifies things I can do that.

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8678258 [details] [diff] [review]
1217935.patch

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

That's true -- I think renaming the 3rd argument in the reducers helps, and maybe just change terminology -- rather than an EventEmitter, maybe focus on "subscribing to changes" rather than "listening to events", so reducers would receive `scheduleChange(eventName, data)`, and on/off are onChange, offChange or something, just to hit home this is not just a normal event emitter. r+, but should consider name changes where appropriate and move away from event emitter terminology

Comment 7

[James Long (:jlongster)]

Attachment: 1217935.patch

Changed the names, now using "state broadcaster" and all the methods are "onChange", "emitChange", etc. I think "emitChange" is different enough. I think I'm the only one using this now so hopefully this will land fine.

Comment 8

[James Long (:jlongster)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef452b112d24

Comment 9

[James Long (:jlongster)]

This bug actually got merged in with bug 1200798 and landed there.

Comment 10

[James Long (:jlongster)]

This bug actually got merged in with bug 1200798 and landed there.