search

stimulusreflex / stimulus_reflex

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

Lifecycle methods only called for one reflex #225

Closed davidalejandroaguilar closed 4 years ago

davidalejandroaguilar commented 4 years ago

Bug Report

Describe the bug

When you setup two reflexes within the same container using the Stimulus data-controller attribute like the docs example, only the first declared reflex lifecycle methods will get called.

In the HTML below, if you click on the TestReflex button, you will trigger the OtherReflex lifecycle method.

<div data-controller="other test">
  <button data-reflex="click->TestReflex#create">TestReflex</button>
  <button data-reflex="click->OtherReflex#create">OtherReflex</button>
</div>

By the way, I encountered this when declaring some generic reflexes / controllers on the body tag, e.g. <body data-controller="debounced ui">. I wanted to use lifecycle methods on one of them but they were not getting called.

To Reproduce

  1. Create an HTML document like this:
<div data-controller="other test">
  <button data-reflex="click->TestReflex#create">TestReflex</button>
  <button data-reflex="click->OtherReflex#create">OtherReflex</button>
</div>
  1. Setup your client side reflexes like this:
// test_controller.js
import ApplicationController from './application_controller';

export default class extends ApplicationController {
  beforeReflex() {
    alert('from TestController beforeReflex');
  }

  beforeCreate() {
    alert('from TestController beforeCreate');
  }
}
// other_controller.js
import ApplicationController from './application_controller';

export default class extends ApplicationController {
  beforeReflex() {
    alert('from OtherController beforeReflex');
  }

  beforeCreate() {
    alert('from OtherController beforeCreate');
  }
}
  1. Setup your ApplicationController like this:
import { Controller } from 'stimulus';
import StimulusReflex from 'stimulus_reflex';

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

  beforeReflex(element, reflex) {
    alert('from ApplicationController');
  }

  reflexSuccess(element, reflex, error) {}

  reflexError(element, reflex, error) {}

  afterReflex(element, reflex) {}
}

Expected behavior

Screenshots or reproduction

Versions

StimulusReflex

External tools

Browser

davidalejandroaguilar commented 4 years ago

Found the issue. I'm opening a PR for this.

davidalejandroaguilar commented 4 years ago

I just noticed this is not a documented use pattern. There are 2 documented ways:

The one I described above was something in the middle:

The gotcha here is the little note on the docs that I didn't see before:

If you're calling the stimulate method inside of a Stimulus controller, the event will be emitted by the element the data-controller attribute is declared on.

Which would mean that, for this middle case, it would trigger the wrong lifecycle method (incorrect controller).

Having said that. I believe this PR is still useful for cases like this where you declare a bunch of controllers on the body or some distant parent; then use data-reflex so as to not having to stimulate manually, but being able to use lifecycle methods.

I think this should also remove that note from the doc.

Let me know what you think.

leastbad commented 4 years ago

Hi David, I sent you a note on Discord but you didn't stick around to see it, I don't think.

I wanted to thank you for filing such an excellent issue (and PR!) and I am sorry that it can take time to get to these as we're all volunteers. I will say that posting a working clonable repo is :100: and the fastest path to success. Kudos.

I am still working through this but I wanted to offer some code review as I hit items of interest.

First, it's probably easier to work through a series of notifications if you send them to console.log() instead of alert().

Second, I noticed that the reflexes were failing on the server. I created empty create actions so that we could eliminate server issues from the conversation.

Third, you're extending ApplicationController, which has a beforeReflex method. If you define beforeReflex in your controller, you're replacing the original. If you want both of them to run, you have to call the ApplicationController method using super.beforeReflex().

I just went through PR #226 and with the minor caveat that we've made the string fragment Reflex that is in every declarative Reflex optional (so this Regex isn't technically valid) I actually like this PR a lot. @hopsoft has to be the final arbiter of whether this is gonna stick the landing, but conceptually I see what you're trying to do and it does seem like we should make an effort to find the right controller if this situation emerges.

Also, before I get ahead of myself, I want to make it super clear that the SR documentation is an evolving, imperfect attempt to capture a deceptively nuanced collaboration. Not only are we constantly tweaking and adding to the documentation, but I often learn corners of the library by doing my best to write about it. This means that some details can slip through that just aren't helpful or presented in the best way possible. This conversation gives me a few places to improve, because there have been some miscommunications. 😀

Having starred at and played with your sample app a lot over the past few days, I think I finally best know how to explain where things are going wrong. Unfortunately, you are currently confusing "not documented" with "not supported", but because of the specific (clever!) way that you're trying to use it improperly, you're seeing results that don't line up with your expectations.

Today, StimulusReflex is intended to be used in one of the two ways we describe: a declared reflex in the markup, or explicitly calling stimulate. The middle path you're describing isn't a feature we're trying to support. Declared reflexes are perfect for things that don't really need callbacks, although they will work in a simple nesting scenario. And as I said earlier when I expressed support for #226, let's be liberal in what we accept when possible.

That said: if you need callbacks, we strongly advise that you call stimulate().

So, what the hell is going on, I feel you asking from here... There's two (maybe three) things that you might not have caught which hopefully demonstrate the method to the madness. The first is that when you put a data-reflex attribute on an element, behind the scenes we're actually putting a Stimulus controller called (wait for it) stimulus-reflex on the element. So, the element that is emitting the event is the element with the data-reflex attribute, not the controller it's nested under. The reason the Stimulus controllers are receiving events is because SR events bubble up from those declared reflex elements.

woah

The final piece of the puzzle is that stimulate() accepts an optional node element parameter that is the element the event will emit from (falling back to this.element). This means that if you put a data-action on the button and called the controller, which called stimulate... the event would come from the controller, not the button.... unless you explicitly specified that the button (or some other element!) should emit it, instead.

To summarize, your other/test controllers are seeing bubbled events because you're currently attempting to make SR work in a way that we didn't intend. You can throw listeners on the data-reflex elements and see for yourself. You can technically use these events if they suit you, but it wasn't what we had in mind and we can't explicitly guarantee that future versions of the library will maintain this behaviour (though you're probably okay?!)

And meanwhile, you did point out a legit way that we could probably improve our edge case handling, and I hope it works out.

Does this all make sense?

davidalejandroaguilar commented 4 years ago

Thank you very much for your response and time @leastbad.

Yes it makes sense. Last night I figured out that I was using it in an unsupported, rather an undocumented way. Your explanation made it click even more, so thanks for that!

I agree that this would be an edge case since there is a correct way of doing things that would prevent this from happening. I guess the PR could help us prevent this and maybe future edge cases related to this, but I'm not sure if the extra loops would be worth it since we're not trying to offer any guarantees around this specific use case.

In the meantime, do you think we should close this issue and leave the PR open for discussion?

leastbad commented 4 years ago

Awesome! I really wanted you to know that you've been properly heard. I thought through everything I said because there's always the chance that we were wrong when we decided how this should work.

Anyhow, if we're settled here, please do close the issue. I'm not in the habit of closing other folks' issues. 👼

As for the PR, let's fix up that Regex and then hopefully @hopsoft loves what he sees!