Setup StimulusReflex controller callbacks

[hopsoft]

Enhancement

Inspired by #39

Demo using this feature. hopsoft/stimulus_reflex_todomvc#27

Description

Adds custom controller callbacks. The hope is to support more sophisticated use cases for complex UIs.

• beforeReflex
• reflexSuccess
• reflexError
• afterReflex

export default class extends Controller {
  connect() {
    StimulusReflex.register(this);
  }

  beforeReflex() {
    // show spinner, do stuff...
  }

  afterReflex() {
    // hide spinner, do stuff...
  }
}

Checklist

• My code follows the style guidelines of this project
• Checks (StandardRB & Prettier) are passing

[leastbad]

All tests were done with current master stimulus_reflex_todomvc + cable_ready, dom-events branch of stimulus_reflex.

1. Tried out stimulus-reflex:before and it worked as expected. It's definitely a step in the right direction. The problem right now is that if you're working with an app where you have multiple instances of a controller defined, how do you know you're the instance that should respond to the event? For example, if I was calling edit, I need my callback to set focus on the correct text element... and even if I only have one text element that matches a selector, I don't need to call .focus() on it more than once.

2. stimulus-reflex:complete and stimulus-reflex:success are not working as expected - no events are captured? I don't have an easy way to test :error... interestingly, putting raise StandardError.new "This is an exception" on the second line of the edit method in the Todos reflex raised the error but did not interfere with the rest of the page rendering and websocket broadcasting. But if I had to make a call, I don't think stimulus-reflex:error is working, either.

3. Edit, cancelEdit, filtering, deleting, deleting completed, toggling all are all working. Creating, updating, toggling are generating JS errors:

4. I would still love to talk about supporting redirects, whether in stimulus_reflex or in cable_ready. Especially redirect support that checks for the presence of Turbolinks and prefers Turbolinks.visit() over window.location.

5. reflexStart and reflexComplete both seemed to trigger successfully on cancelEdit but not the initial edit. I suspect that this is because cancelEdit is the only reflex called via an action handler and the stimulate() method. This seems backed up by the toggle_all and filter functions also not triggering the callbacks. This only really sizzles if it works for declarative reflexes as well.

6. These callbacks are called for every reflex method (or at least they will hopefully soon!) which means that the developer will have to put a switch statement into the function to first figure out which method the callback is for.

As currently implemented, I don't see a lot of value over just using the event capture and interrogating the details. They just make the API bigger.

The reason I like the onEdit (and you've given me a great idea in the form of beforeEdit, too!) is that the purpose of the callback is explicit and you don't have an ugly control structure.

I believe that if we have the information needed to create the reflexStart and reflexComplete callbacks, we also have the information needed to create onEdit and beforeEdit method pairs for all of the events. If you look at my PR, you see that I just assemble the potential function names as strings, test to see if they are defined as functions and if they are, I execute them via the object[] notation.

For the price of a few lines of JS in the library, every dev gets to use really amazing syntax.

¯\_(ツ)_/¯

FWIW, I feel like I sound harsh and negative. I'm really excited for this! I just want to get this right, and I am thorough.

[hopsoft]

@leastbad The last commit should somewhat address why things weren't working for you. It will at least provide enough context to help developers work around any problems similar to what you encountered.

The issue was related to the DOM selector that gets stitched together when an id isn't present. Apparently document.querySelectorAll('[data-foo.bar="baz"]') fails as a syntax error even though dots are legal chars in the attribute name. At any rate this is a simple enough fix and should help guide developers to a simple solution.

[hopsoft]

To test the error condition, simply raise in one of the Ruby reflex methods.

[andrewmcodes]

Overall I love this. I think it makes a lot of sense and is pushing this library forward in a great way. Most of my comments could be taken as questions or nitpicks but felt it was worth bringing them up. As far as naming, I think they all make sense. Is there documentation we could update? That stood out to me above everything else.

[andrewmcodes]

I like dispatch much better. Happy to set up a time to pair on tests. Just ping me on Slack.

[leastbad]

I finally had a chance to check out the latest and greatest for both stimulus_reflex and stimulus_reflex_todomvc. I watched the video, too. That was some seriously hi-res video! 111MB/26s is wild... when I download a 720p show it's usually 350MB or so for ~23 minutes. I digress! Did you notice some strange artifacts when you clicked the toggle buttons?

For some reason, I can't actually get animate.css to work so my build is broken. I'm going from memory and the video, which sucks a bit. Not sure what's up, but my todomvc currently has no styles and there's no ActionCable WS loading... and no errors on bin/webpack or on my Chrome console. Annoying!

Looking at the todomvc example, it seems as though most of what you've done is represented in your custom default_controller.js, correct?

Let me start by saying that I definitely appreciate seeing some solid examples of the kinds of thing that really does belong in the stimulus-reflex controller, and benefit from the callback system as it is currently proposed/implemented.

The best example is almost certainly the way you toggle the wait class on the body element. While hopefully most people never see a spinner because it's just that fast, this is a really clever implementation of a feature that really should be there, especially if you're calling long-running tasks like API calls on the server.

The only problem I see here is that, depending on how things are implemented, it's entirely possible that the wait class would be over-written as soon as the user types or clicks on something else while the long-running call is still waiting. I could make a pretty good argument that the spinner is actually something that should be implemented with the template and an instance variable, because otherwise what happens when they refresh or load a second duplicate tab?

BTW: the trick you used to place the cursor at the end of the autofocus text element is both a ridiculous hack and by far the most elegant of all the many ridiculous hacks I've seen. Keeping that one around for later...

This is where I switch into constructive feedback mode, though. I am sincerely hoping that we can get on the same page about most if not all of the following, especially now that you've put some energy into implementing some basic UI feedback with the current API.

First, while I agree that there probably are uses for a global, default controller (and it would appear to impose a 0 LOC cost on the project) I continue to feel strongly that the only correct place for callbacks initiated by actions associated with the todos controller is in the todos_controller.js. If I need to handle autofocus after calling "click->TodosReflex#edit" then I'm going to go look at the todos controller, not some abstract generic class that only exists because we added support for declarative reflexes a few weeks ago.

From a code modularity and developer happiness perspective, having all events in your entire application handled in a generic controller reminds me of the scene in Fantasia when a well-intentioned Mickey Mouse starts summoning more and more dancing buckets and everyone just goes to hell on a road paved with good intentions.

I would like very much to be able to mix-and-match which controllers I use on different views in my application, without bringing in global event code that gets ever more complex to handle different weird edge cases for individual screens. And that's assuming I'm the only one working on it! Junior dev nightmare flashbacks.

Second, if you look at your reflexSuccess() example, you've already got two distinct logic branches which have very little to do with each other; a test for autofocus and a test for checkboxes... and this is for a tiny demo.

Not only should every controller be able to handle its own callbacks, but the most important distinction is being lost when we put the primary focus on the class callback Success event. The action that triggered the success is just as important and first-class as the success flag. This is cutting to the heart of what I tried to raise in what I labelled as issue 6 in my previous comment.

At full risk of flogging a horse, in my preferred and proposed API, you would instead see:

todos_controller.js

onEdit() {
  // code to handle autofocus after they "click->TodosReflex#edit"
}

onToggle() {
  // code to add animation class after they "click->TodosReflex#toggle"
}

You also managed to accidentally get me excited about supporting beforeEdit() and beforeToggle() as well. There should probably be onEditError(), too.

If you look at the three supporting functions in your controller (isCheck, listItemTarget, headerTarget) they are each dedicated to either finding the correct element to work on or validating that a path on your reflexSuccess logical tree should be followed. In other words, they are pure ceremony and don't add functionality, just boilerplate.

Just to be clear: I'm glad that the 4 class-level events are there and think they are super handy to expose in addition to the action-based events I'm advocating strongly for. If todos initiates a reflex action, it completely makes sense for todos to receive reflexSuccess events pertaining to it that will not be captured by my users, clients or invoices controllers. And by extension, it makes sense that the custom stimulus-reflex override controller could define a reflexError() event handler that picks up any errors for all controllers. Heck, the custom override controller could even define an onEdit() callback that would get fired for all controllers, too.

But the hierarchy of information flow is really important when considering building a real-world app with the Reflex/Live View pattern. I gave you a working implementation of the proposed API and if you strip out the redirects and ability to define which callback you want in the reflex action, you don't have to make any changes to the Ruby lib at all. We have the controller and action available in the event.details, we just have to do a bit of split+toLowerCase+append magic to get the exact API I proposed. You can test the controller to see if the callback is defined, and if it is, you run it. Without the added complexity of parsing out redirects and overridden callbacks from the server, the actual implementation would be 6-10 lines.

I'd also still like to discuss items 4 and 5, although I recognize that perhaps 4 should be a separate PR that we sort out after callbacks. As for 5, given that my build just won't let me actually click on anything right now, can you address the question of whether these callbacks work for both declarative reflexes as well as explicit calls to stimulate()?

[hopsoft]

Pretty sure we share similar opinions regarding how to best architect a StimulusReflex project.

Having said that, I'd also stress that StimulusReflex is a lower level utility and I don't feel it should impose a particular style... even though there are "best practices" that we can and should document. This is similar to Rails itself.

---

I don't agree that more specific events like edit and toggle belong in the library. These are simple enough use cases that can be built on top of the existing lib without much effort by application developers. For example,

1. Define a custom StimulusReflex controller
2. Do typical Stimulus things to handle user interaction(s)
3. Setup an explicit call to stimuluate
4. Implement reflexBefore and reflexComplete

[leastbad]

I think that there's been a [awesome] misunderstanding.

My PR dynamically looks up and calls those events based entirely on the name of the action.

The entire implementation is apparently 8 lines, depending on how you count it.

Swap out line 40... we can just take event.details.target.split('#')[1] instead.

Similarily, swap out line 32 so the controller reference comes from event.details.target.split('#')[0].

No changes to the Ruby lib required.

[I think it's event.details.target, but I don't have a working build right now. I think you know what I mean, though... the string with the "TodosReflex#action" in it.]

Anyhow, at no point have I ever wanted or attempted to imply that we should hard-code any action-based event handler into the library. I passionately hope that this triggers a moment of 💡 ⚡

[hopsoft]

I think that there's been a [awesome] misunderstanding.

My PR dynamically looks up and calls those events based entirely on the name of the action.

Indeed! This is a pretty slick convention. 🤔 Will think on this some more.

[leastbad]

This looks really great so far, Nate. Thrilled to see it moving in this direction.

I'm going to kick the tires in my master branch of todomvc.

[leastbad]

This invokeCallback probably should have a different name because it's useful for all sorts of things that aren't callbacks. Can we call it send as a homage to Ruby?

Is send a valid function name in JS?

Probably not super important, but technically we could change the name of the controller parameter to class (or klass) because really this is a cool pattern even if you're working in React. I always like the idea that people reading the code can learn new tricks, and this isn't specifically a Stimulus thing.

Of course, it might just be confusing, too. I'm not hung up either way.

[hopsoft]

You're right that it's something of a misnomer to name it a callback. I'll take some time to consider whether or not there is a better naming convention we can use.

[leastbad]

I tried to kick the tires and I'm sad to say that I can't get the callbacks working. I'm sure it's something silly. I'm using the master branch of stimulus_reflex_todomvc, the only changes being that the gem points to my local checkout of your dom-events stimulus_reflex branch. The npm module is also linked to your dom-events branch. I have verified that things are as expected by inserting a console log in the stimulus_reflex.js which is executing.

If you take the stock todos_controller.js and add the following below the cancelEdit definition:

  beforeEdit (arg) {
    console.log('before edit', arg)
  }

  afterEdit (arg) {
    console.log('after edit', arg)
  }

  editSuccess (arg) {
    console.log('success', arg)
  }

  editError (arg) {
    console.log('error', arg)
  }

None of the callbacks are firing, including editError if I make the reflex class raise a bogus error on the server.

I'm clicking on the first todo item in the list, which is definitely firing an edit action based on the server log tail.

This should be super vanilla. Any ideas why it doesn't seem to work?

[hopsoft]

@leastbad Make sure you do the following during development.

1. cd /path/to/this/project
2. git checkout hopsoft/dom-events
3. gem uninstall stimulus_reflex
4. rake install
5. cd javascript
6. yarn link
7. cd /path/to/todos/project
8. bundle update
9. yarn link stimulus_reflex

Remember that the only event the todos controller handles (on the master branch of the todos demo) is cancelEdit.

Note also that args are no longer passed to the callbacks.

[leastbad]

Just getting this on my phone, looking forward to trying later.

As I said: I’ve verified that I have my reflex gems and packages properly redirected.... otherwise, i would have had serious issues doing anything previously as well.

I honestly wasn’t sure what, if anything, would arrive as an argument; I see now that we’re just calling the functions straight.

The only thing that seems off is your comment about how the only event todos handles is cancelEvent... I’m using dom-events branch and defining the callbacks I want for todos actions. What am I missing? In other words, where else would they be? Or put more pointedly: this is the only place that they should be...

[hopsoft]

Bear in mind that this PR is still WIP. I have more changes coming tonight.

[leastbad]

Observations about the current builds of todomvc:lifecycle-demo + SR:dom-events.

First thing: I realized last night that the reason there's no connect() + register() in todos_controller is that it inherits from ApplicationController. Duh!

I put a console log in beforeReflex() in application_controller. I added a beforeReflex with a console log in the todos controller. When I initiate a reflex (edit, cancelEdit or Toggle) I see the log generated by the beforeReflex() in todos but not the one from application_controller.

If I then remove the beforeReflex() from todos, the one in application_controller fires.

If don't know if this is intended functionality. You were talking about cancelling propagation although I don't see any evidence that you've implemented this in the library or the demo. [I wrote this before I realized the part about extending ApplicationController, above. But this still leads to a solid point.]

The problem I anticipate here is this is another example of possible "cross-talk"; someone joins my team and creates a chill controller that defines a beforeReflex() and suddenly the critical code we rely on that lives in application_controller is "broken" even though they didn't change any code. The new guy has no idea that he's created a side-effect.

It seems like the beforeReflex and afterReflex callbacks should fire on all controls that define them, including application_controller. Thinking about it, I don't have a strong opinion of whether to run them on the application or individual controllers first. I'm sure that there's a right answer.

(I include the reflexSuccess and reflexError callbacks in the above as well.)

Addendum: I realize now that the reason beforeReflex() isn't calling twice is that Todos is redefining the definition of the beforeReflex function after inheriting from ApplicationController. However, the larger issue remains: what if some joker creates a controller with a beforeReflex function in it, cancelling the version that's in ApplicationController? I honestly don't know how to fix this beyond putting it in the documentation that if they define a beforeReflex in any one of their controllers, it will clobber the one in ApplicationController, likely pissing someone in the office right off.

QQ: will reflexError get access to the error message from a failed reflex on the server?

Not so QQ: should you be able to return false from a beforeReflex or beforePoop and have it cancel sending the reflex entirely?

Is there a way to inject a time delay via the beforeReflex or beforePoop callback wraps up and the reflex proceeds in sending data to the server? In Ruby we have a blocking sleep call. JS has a non-blocking async setTimeout which won't prevent the callback from returning before the timeout returns. It's actually a really thorny issue but I don't want to overthink.

I was/am a little worried that we might need/want a blacklist of methods we don't want to allow reflexes for eg. def send proc lambda method_missing although I'm currently testing an InvoiceReflex#send that seems to work just fine. I guess you can .send(:send) in this crazy world.

Anyhow, great news... I have added this to my index.html.erb:

<section data-controller="invoice">
  <a href="#" data-reflex="click->InvoiceReflex#send" data-room="<%= session.id %>"/>Send Invoice</a>
</section>

When I click it, only the invoice controller calls its' beforeSend() callback. And I don't see any callbacks for todos being called inside of invoice. I think that you've nailed the problem. Great work.

[hopsoft]

I put a console log in beforeReflex() in application_controller. I added a beforeReflex with a console log in the todos controller. When I initiate a reflex (edit, cancelEdit or Toggle) I see the log generated by the beforeReflex() in todos but not the one from application_controller.

OOP is only one pattern people might use. I think it's reasonable to expect that they understand the repercussions and constraints if they choose to use it. When using inheritance, the appropriate way to get the desired behavior would be to call the super's method.

// app/javascript/controllers/application_controller.js
import { Controller } from 'stimulus';
import StimulusReflex from 'stimulus_reflex';

export default class extends Controller {
  connect() {
    StimulusReflex.register(this);
  }

  beforeReflex() {
    document.body.classList.add('wait');
  }
}

// app/javascript/controllers/todos_controller.js
import ApplicationController from './application_controller';

export default class extends ApplicationController {
  beforeReflex() {
    super.beforeReflex();
    // do other things
  }
}

I think this is something that should be documented rather than adding mechanics to protect the developer from themselves.

---

will reflexError get access to the error message from a failed reflex on the server?

I supported this when this PR was dispatching events. I'll update to send the error message into error based lifecycle methods like reflexError. The method signature will be

reflexError(sourceElement, errorMessage) { ...

---

should you be able to return false from a beforeReflex or beforePoop and have it cancel sending the reflex entirely

Possibly... I like this thinking; however, I also want consistency across all lifecycle methods. Not sure what we'd cancel if the others returned false.

---

Is there a way to inject a time delay via the beforeReflex

I've though about this for handling things like UI animations (fade & hide on delete etc...) with a promise that invokes the reflex when the animation completes. Slowing things down to create a better perceived speed and/or improve the UX. I think we should support it (make it optional), but doubt I'll add it to this PR.

[leastbad]

I'm a little late to the party, but I thought you guys would get a kick out of this:

https://elixirforum.com/t/phoenix-liveview-callbacks-to-put-text-input-focus-on-the-selected-input-field/21148/7

https://elixirschool.com/blog/live-view-with-channels/

Can I just say again that I love the API we came up with? It kind of warms my heart that this is impossible to do in LiveView.