Make redux reducer events fire at the right time

RESOLVED INVALID

Status

RESOLVED INVALID
3 years ago
2 months ago

People

(Reporter: jlong, Assigned: jlong)

Tracking

40 Branch

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8678255 [details] [diff] [review]
1217935.patch

Jordan I don't think are using this because you are using React, but if you are the API has changed.
Attachment #8678255 - Flags: review?(jsantell)
(Assignee)

Comment 2

3 years ago
Created attachment 8678258 [details] [diff] [review]
1217935.patch
Attachment #8678255 - Attachment is obsolete: true
Attachment #8678255 - Flags: review?(jsantell)
Attachment #8678258 - Flags: review?(jsantell)
(Assignee)

Updated

3 years ago
Assignee: nobody → jlong
Blocks: 1177836
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?
(Assignee)

Comment 4

3 years ago
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.
(Assignee)

Comment 5

3 years ago
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 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
Attachment #8678258 - Flags: review?(jsantell) → review+
(Assignee)

Comment 7

3 years ago
Created attachment 8685617 [details] [diff] [review]
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.
(Assignee)

Comment 9

3 years ago
This bug actually got merged in with bug 1200798 and landed there.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 10

3 years ago
This bug actually got merged in with bug 1200798 and landed there.
(Assignee)

Updated

3 years ago
Resolution: WONTFIX → INVALID

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.