Using action option changes the order of actions

janko commented 1 year ago

In certain situations, using an action option such as :prevent can change the order in which actions registered in the same data-action are executed.

Here is a self-contained example demonstrating the problem:

<html>
  <head>
    <script type="module">
      import { Application, Controller } from "https://unpkg.com/@hotwired/stimulus/dist/stimulus.js"
      window.Stimulus = Application.start()
      Stimulus.debug = true
      Stimulus.register("one", class extends Controller {
        foo() {}
      })
      Stimulus.register("two", class extends Controller {
        bar() {}
      })
    </script>
  </head>
  <body>
    <div data-controller="two">
      <div data-controller="one">
        <a href="#" data-action="one#foo two#bar">Click me</a>
      </div>
    </div>
  </body>
</html>

Without the action option, the debug output shows the correct order:

one #foo
two #bar

However, if I use an action option:

<a href="#" data-action="one#foo:prevent two#bar">Click me</a>

The debug output shows the order in which actions are executed has changed:

two #bar
one #foo

This seems like a bug to me. I encountered it in a scenario where the order of actions was important, and it took me a while to realize it's because of the action option.

adrienpoly commented 1 year ago

Thanks @janko for this detailed report. I think this PR https://github.com/hotwired/stimulus/pull/684 might fix it.

The PR mentions action added dynamically so it is not exactly the same. I ll see if I can create a test reproducing your but and run it against that PR.

adrienpoly commented 1 year ago

I ran some additional test

given this original test from the test suite:

async "test adding an action to the left"() {
    this.actionValue = "c#log3 c#log d#log2"
    await this.nextFrame
    await this.triggerEvent(this.buttonElement, "click")

    this.assertActions(
      { name: "log3", identifier: "c", eventType: "click", currentTarget: this.buttonElement },
      { name: "log", identifier: "c", eventType: "click", currentTarget: this.buttonElement },
      { name: "log2", identifier: "d", eventType: "click", currentTarget: this.buttonElement },
      { name: "log", identifier: "c", eventType: "click", currentTarget: this.element }
    )
  }

I modified it to add a :prevent modifer

async "test adding an action to the left with a :prevent modifier"() {
    this.actionValue = "c#log3:prevent c#log d#log2"
    await this.nextFrame
    await this.triggerEvent(this.buttonElement, "click")

    console.log(this.actionLog.map(({ name }) => name))
    this.assertActions(
      { name: "log3", identifier: "c", eventType: "click", currentTarget: this.buttonElement },
      { name: "log", identifier: "c", eventType: "click", currentTarget: this.buttonElement },
      { name: "log2", identifier: "d", eventType: "click", currentTarget: this.buttonElement },
      { name: "log", identifier: "c", eventType: "click", currentTarget: this.element }
    )
  }

The test fails events are received in this order ['log', 'log2', 'log3', 'log'] whereas it should be ['log3', 'log', 'log2', 'log']

Running the same test with #684 works. I'll what we can do to get this PR merged. While it is not a complete solution think it solves the vast majority of this issue

hotwired / stimulus

Using action option changes the order of actions #718