Closed Bug 1217935 Opened 9 years ago Closed 9 years ago

Make redux reducer events fire at the right time

Categories

(DevTools :: Debugger, defect)

40 Branch
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: jlong, Assigned: jlong)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached patch 1217935.patch (obsolete) — Splinter Review
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)
Attached patch 1217935.patchSplinter Review
Attachment #8678255 - Attachment is obsolete: true
Attachment #8678255 - Flags: review?(jsantell)
Attachment #8678258 - Flags: review?(jsantell)
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?
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.
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+
Attached patch 1217935.patchSplinter Review
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.
This bug actually got merged in with bug 1200798 and landed there.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
This bug actually got merged in with bug 1200798 and landed there.
Resolution: WONTFIX → INVALID
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: