search

hotwired / stimulus

A modest JavaScript framework for the HTML you already have
https://stimulus.hotwired.dev/
MIT License
12.73k stars 426 forks source link

Action parameters do not work if the controller identifier has any capital letter in it #561

Closed asprega closed 2 years ago

asprega commented 2 years ago

Hi Stimulus team, and thanks for your amazing work.

Sorry for not having replied on time, but referring to my previous issue: https://github.com/hotwired/stimulus/issues/558 I think I did not explain clearly my problem, so I will try to do it now.

Problem: when the controller identifier has any capital letter in it, params are not passed to the triggered action.

This is a code snippet that reproduces the issue:

<html>
<head>
  <script type="module">
    import { Application, Controller } from "https://cdn.skypack.dev/@hotwired/stimulus@3.0.1";

    const application = Application.start();

    class CamelCaseController extends Controller {
        action(event) {
            alert(event.params.foo); // Opens alert with `undefined`
        }
    };

    application.register("CamelCase", CamelCaseController);
  </script>
</head>
<body>
 <div data-controller="CamelCase">
   <button data-action="CamelCase#action"
        data-CamelCase-foo-param="bar">
     Open alert
   </button>
 </div>
</body>
</html>

The result is that undefined is printed, even though the parameter foo should have value bar

If you change the controller name to camelcase, thus removing capital letters both in html and js, things will work as documented.

I believe this is an issue because other features of the framework (i.e. targets and values) work even when the controller identifier is not all lowercase.

If you confirm this is something to be fixed, I can open a PR for this.

Thanks!

seanpdoyle commented 2 years ago

Regardless of whether they're in [data-action], [data-controller], or [data-*-target] attributes, Controller identifiers must be lower case and their words must be - separated.

If you replaced all instances of CamelCase with camel-case and data-CamelCase-foo-param with data-camel-case-foo-param, does it behave as you expect?

asprega commented 2 years ago

Thanks for your reply!

If I do that find/replace both in the controller identifier and the data attribute then yes, it does behave as expected. After all, at that point it's the basic scenario the documentation talks about.

The reason why we're using controller identifiers with capital letters is because we're using Typescript classes and we like to keep a consistent naming between the class name and what we see in the HTML template, so that there's a nice 1:1 mapping and no indirection.

This works well everywhere ([data-action], [data-controller], [data--target], [data--value]), only with params it does not (because there the controller identifier matching is not case insensitive). This is an inconsistent behavior, and I believe this should either be fixed or maybe it should be stated clearly in the documentation? The documentation, while suggesting identifiers with lowercase and hyphens/underscores, doesn't seem to say that identifiers with capital letters won't work (and in fact they do, except for [data-*-param]).

What do you think?

adrienpoly commented 2 years ago

I agree that it should be consistent either full support or not . Since this was the original behavior for other attributes, we should update the params API with that case insensitive regex. This being said, I don't think this should be documented

PauchardThomas commented 2 years ago

I got the issue with an upperCase folder inside controllers :

brunoprietog commented 2 years ago

It is easier to follow conventions