Closed
Bug 1217935
Opened 9 years ago
Closed 9 years ago
Make redux reducer events fire at the right time
Categories
(DevTools :: Debugger, defect)
Tracking
(Not tracked)
RESOLVED
INVALID
People
(Reporter: jlong, Assigned: jlong)
References
Details
Attachments
(2 files, 1 obsolete file)
6.41 KB,
patch
|
jsantell
:
review+
|
Details | Diff | Splinter Review |
7.42 KB,
patch
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
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•9 years ago
|
||
Attachment #8678255 -
Attachment is obsolete: true
Attachment #8678255 -
Flags: review?(jsantell)
Attachment #8678258 -
Flags: review?(jsantell)
Comment 3•9 years ago
|
||
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•9 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•9 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 6•9 years ago
|
||
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•9 years ago
|
||
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 8•9 years ago
|
||
Assignee | ||
Comment 9•9 years ago
|
||
This bug actually got merged in with bug 1200798 and landed there.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Assignee | ||
Comment 10•9 years ago
|
||
This bug actually got merged in with bug 1200798 and landed there.
Assignee | ||
Updated•9 years ago
|
Resolution: WONTFIX → INVALID
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•