stimulusreflex / stimulus_reflex

Build reactive applications with the Rails tooling you already know and love.
https://docs.stimulusreflex.com
MIT License
2.28k stars 172 forks source link

too many afterReflex/reflexSuccess callbacks #68

Closed leastbad closed 5 years ago

leastbad commented 5 years ago

Bug Report

Describe the bug

If a reflex is scoped to update multiple selectors, the reflexSuccess and afterReflex callbacks are being executed once per morph operation instead of once per cable_ready payload.

To Reproduce

Ensure afterReflex writes to the console log. Call a reflex that is scoped to two or more selectors.

Expected behavior

Each reflex action should fire callbacks one time.

Versions

StimulusReflex

External tools

Browser

hopsoft commented 5 years ago

One possible solution. Generate a UUID that uniquely identifies the reflex invocation and send this id as part of the data payload here: https://github.com/hopsoft/stimulus_reflex/blob/master/javascript/stimulus_reflex.js#L135

Then use a unique/debounce type strategy to ensure after/success invocations only happens once for that UUID. The trick will be to ensure we only fire after the last one.

leastbad commented 5 years ago

I've been starring at this for a while. It is a surprisingly thorny problem to "solve" as the desired outcome might be different for two developers.

From the perspective of cable_ready, each one of these updates are atomic and call their own before-morph/after-morph events. The issue is that I can picture scenarios where the developer absolutely wants a succession of serialized results - in particular, the attached meta-data, which will be different for each operation.

The fact that our applications currently want to be notified before the first morph and after the last morph is an implementation detail. It's not a more or less important use case.

There are definitely some strategies that we can explore, some more elegant than others.

We could add some additional metadata to each operation, for example we could grab a count of the total number of operations and then pass a two-dimensional array such as [1, 5] which would suggest that this is the second item out of a batch of six. Or more simply, we could have firstOperation:true and lastOperation:false as parameters, though I like the explicit nature of [x,y] better.

This metadata could be accessed in the callback through the function arguments and used to construct a guard. It's not super elegant.

On the same wavelength, instead of forcing people to write guards everywhere, we could add two additional global callbacks: beforeAllMorphs/afterAllMorphs and those could become the place where things like spinners and benchmarking could go, leaving the original callbacks for scenarios where the developer wants to operate on a set of changes.

Having the client SR library essentially counting events like a bouncer with a clicker seems like a kluge at best, unfortunately.

hopsoft commented 5 years ago

I've been thinking about this issue for a while and here's where I've landed.

Triggering a single event for success|after should suffice for the vast majority of use cases.

I propose we punt on the idea of triggering events for each individual morph until a solid use case forces the feature.

hopsoft commented 5 years ago

Note that adding an event for each morph will be trivial if/when we deem it necessary.

leastbad commented 5 years ago

I haven’t had a chance to eyeball the code, yet, but I’m happy with your logic and reasoning.