Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
asprega opened this issue Jul 18, 2022 · 5 comments · Fixed by #571
Closed

Comments

@asprega
Copy link

asprega commented Jul 18, 2022

Hi Stimulus team, and thanks for your amazing work.

Sorry for not having replied on time, but referring to my previous issue: #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/[email protected]";

    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
Copy link
Contributor

seanpdoyle commented Jul 18, 2022

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
Copy link
Author

asprega commented Jul 18, 2022

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
Copy link
Member

adrienpoly commented Jul 18, 2022

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
Copy link

PauchardThomas commented Jul 19, 2022

I got the issue with an upperCase folder inside controllers :

  • assets/controllers/adminArea/orders_controller.js -> params not working
  • assets/controllers/adminarea/orders_controller.js -> params working

@brunoprietog
Copy link
Collaborator

It is easier to follow conventions

marcoroth added a commit to marcoroth/stimulus that referenced this issue Aug 9, 2022
Even though this is not the "recommended way" to specify the data 
attributes for the params it's to match the existing behaviour of 
Actions, Controllers, Targets and Values.

Resolves hotwired#561
marcoroth added a commit that referenced this issue Aug 30, 2022
Even though this is not the "recommended way" to specify the data 
attributes for the params it's to match the existing behaviour of 
Actions, Controllers, Targets and Values.

Resolves #561
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

5 participants