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

AlpineJS specific attributes are not rendered (@click and x-on:click) #1839

Closed
loevgaard opened this issue May 10, 2024 · 26 comments · Fixed by #2328
Closed

AlpineJS specific attributes are not rendered (@click and x-on:click) #1839

loevgaard opened this issue May 10, 2024 · 26 comments · Fixed by #2328

Comments

@loevgaard
Copy link

I love the components, so thank you <3

I have a button component and I would like to be able to do:

<twig:Button value="Toggle" x-on:click="open = !open"/>

but the x-on:click attribute is not rendered with this Twig:

<button{{ attributes }}>{{ value }}</button>

instead it just strips the attribute and outputs:

<button>Toggle</button>

If I try to use the @click syntax, Twig throws an exception.

@norkunas
Copy link
Contributor

I think this collides with newly implemented nested attributes.
In your case it would be accessible with attributes.nested('x-on') which in your case is unexpected :)

@kbond
Copy link
Member

kbond commented May 10, 2024

Ah yes, I think @norkunas is correct. Anyone have any thoughts on how to solve?

If I try to use the @click syntax, Twig throws an exception.

I think this could be a html-twig component parsing issue.

@smnandre
Copy link
Member

What does the expection say ? In the end i'm not surprised as it's not valid HTML syntax. But if we can improve this let's do it :)

@loevgaard
Copy link
Author

I think this collides with newly implemented nested attributes.
In your case it would be accessible with attributes.nested('x-on') which in your case is unexpected :)

Now that you pointed me in the right direction, it's totally fine for me :)

I will let you guys close the issue if you believe this is not something you want to 'fix'. For me, when I know what I know now, it's perfect :)

Once again, thanks for a great component!

@WebMamba
Copy link
Collaborator

I think this is legit issue we should keep it open. I don't really know how to solve it... Should we detect alpine attribute? Or should we add a prefix so that the attributes are ignore by the nested? Should we add a prefix for every nested attribute like <twig:Form twig:row:label='password' />?

@loevgaard
Copy link
Author

loevgaard commented May 13, 2024

I don't know if it's possible, but there's also the option to use another separator for nested attributes, e.g. dot:

<twig:Button value="Toggle" nested.attribute="Value"/>

But then again, that might conflict with something else...

@WebMamba
Copy link
Collaborator

But then again, that might conflict with something else...

Yes this is my worries!

@kbond
Copy link
Member

kbond commented May 13, 2024

Yeah, no matter what syntax we use, the possibility for conflicts exists. Some quick ideas:

  1. Make the nested separator configurable
  2. Add a prefix exclude-list
  3. Some kind of escape syntax (ie !x-on:click)

1 & 2 above don't address the issue of using components from 3rd party bundles that would use the default syntax (and be unaware of your configuration). 3 feels like the best but introducing a new syntax isn't ideal.

Now that you pointed me in the right direction, it's totally fine for me :)

I'm glad there's a workaround but still, it's a bit of a bummer. I agree, let's keep this open.

@loevgaard
Copy link
Author

The practical approach would be to do the research of the top 10-20 JS libraries that use this kind of syntax and see how they do it and make an informed decision based on that. Then at least it would hurt the least amount of people. Just thinking out loud :)

@WebMamba
Copy link
Collaborator

This can be a good solution, but some librairies can have conflic, and all of this convention could lead to limitation to the user. So I am not sure about this solution as well

@smnandre
Copy link
Member

Do we accept "dashes" in the component names ? Maybe we could say if "x-...:" it is not a nested param ?

@loevgaard
Copy link
Author

loevgaard commented May 15, 2024

With the knowledge of nested attributes I just tried this:

<twig:Button x-on:click="hidden = false" value="Test"/>

and this

<button{{ attributes.nested('x-on') }}>{{ value }}</button>

and it renders as

<button click="hidden = false">Test</button>

which made me read the docs, and of course it makes sense, but now the user experience of adding these AlpineJS directives just got a bit worse in my opinion. Now I have to prepend x-on: to every value in the attributes.nested('x-on') collection.

Here's my code to do that:

{% set xOnAttributes = attributes.nested('x-on') %}
{% set xOn = '' %}
{% for attr, value in xOnAttributes %}
    {% set xOn = ' x-on:' ~ attr ~ '="' ~ xOnAttributes.render(attr) ~ '"' ~ xOn %}
{% endfor %}

This is pretty ugly :D

Ended up making a twig filter like this:

<?php

declare(strict_types=1);

namespace App\Twig\Extension;

use Symfony\UX\TwigComponent\ComponentAttributes;
use Twig\Extension\AbstractExtension;
use Twig\TwigFilter;

final class Extension extends AbstractExtension
{
    /**
     * @return list<TwigFilter>
     */
    public function getFilters(): array
    {
        return [
            new TwigFilter('nested_attributes', $this->nestedAttributes(...), ['is_safe' => ['html']]),
        ];
    }

    public function nestedAttributes(ComponentAttributes $attributes, string $namespace): string
    {
        $attributes = $attributes->nested($namespace);

        $output = '';

        foreach ($attributes as $attr => $_) {
            $output .= sprintf(' %s:%s="%s"', $namespace, $attr, $attributes->render($attr));
        }

        return $output;
    }
}

then I can output the x-on nested attributes like this:

<button{{ attributes|nested_attributes('x-on') }}>{{ value }}</button>

To make attributes.nested('x-on') the way I first wanted it to, we could change the signature of the method like this:

public function nested(string $namespace, bool $includeNamespace = false): self

if $includeNamespace is true we will preprend the namespace when creating the new list of attributes.

@smnandre
Copy link
Member

I don't think we will change the nested contract.

But, i checked, you cannot create a blog foo-bar, so i really think we could change the nested regex to consider x-on as an attribute and not a nested prop 🤷

@smnandre
Copy link
Member

@kbond would that be ok for you ?

@kbond
Copy link
Member

kbond commented Jul 4, 2024

Totally agree that - should be included in the regex!

As a solution to this issue, another idea: :: to escape? x-on::click renders as x-on:click?

@kbond
Copy link
Member

kbond commented Jul 4, 2024

I believe #1959 & #1960 together would resolve this issue.

@loevgaard
Copy link
Author

You're the best, @kbond. Thank you 🎉

@kbond
Copy link
Member

kbond commented Jul 4, 2024

@loevgaard are you ok with the escape solution for :?

@loevgaard
Copy link
Author

First off I thought it was great, but now I just gave it some thought, and it might be weird for people (not knowing about this PR and issue) that there are two :: when the syntax is one.

However, I think when #1960 is done, that solution is what most (if not all) will resort to. I think I used x-on: because @ didn't work. You can read about the x-on here: https://alpinejs.dev/directives/on. Maybe the 'correct' solution would be to make @ and tell AlpineJS users that they have to use @?

@kbond
Copy link
Member

kbond commented Jul 4, 2024

First off I thought it was great, but now I just gave it some thought, and it might be weird for people (not knowing about this PR and issue) that there are two :: when the syntax is one.

I added a note to the doc but this is a fair critique. The addition of "more hardcoded syntax" isn't ideal to begin with...

Maybe the 'correct' solution would be to make @ and tell AlpineJS users that they have to use @?

Hmm, are there advanced use cases where the @ cannot be used?

@loevgaard
Copy link
Author

Hmm, are there advanced use cases where the @ cannot be used?

Not that I know of, but I am not an expert. I reached out to @jasonlbeggs. If he has time, he might give us some input.

@jasonlbeggs
Copy link

In my experience, x-on and @ are pretty much always interchangeable. When using Alpine with Laravel Blade, there are occasionally conflicts using the @ syntax since Blade uses @ for it's directives.

Long story short, I can't think of any downsides to supporting @ prefixes on attributes and using that syntax for Twig components.

@kbond
Copy link
Member

kbond commented Jul 4, 2024

Thanks @jasonlbeggs!

So perhaps let's roll with just #1960 to consider this issue fixed?

@loevgaard
Copy link
Author

So perhaps let's roll with just #1960 to consider this issue fixed?

Yes, I do think so, Kevin. Thank you!

kbond added a commit that referenced this issue Jul 6, 2024
…(kbond)

This PR was merged into the 2.x branch.

Discussion
----------

[TwigComponent] allow attributes to be prefixed with `@`

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| Issues        | Fix (partial) #1839
| License       | MIT

Allow attributes to be prefixed with `@` (like ``@click`="!open"`).

Commits
-------

c567cbb fix: allow attributes to be prefixed with `@`
@kbond
Copy link
Member

kbond commented Jul 6, 2024

Big thanks to @smnandre for fixing the @ issue in #1966! As discussed, we'll forgo escaping : for now.

@kbond kbond closed this as completed Jul 6, 2024
@ottaviano
Copy link

hey guys, what do you think about this ?

<twig:Button x-bind:class!="hey">My button</twig:Button>

#2325

Kocal added a commit that referenced this issue Nov 23, 2024
… (smnandre)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[TwigComponent] Ignore "nested" for Alpine & Vue attributes

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Issues        | Fix #1839
| License       | MIT

Have lost time on Twig & the website 😓

Alternative implementation of #2325

Update: Improved `__toString` performance following `@Kocal` comments

---
Now all these attributes are directly rendered

| Framework       | Prefix | Code Example                                                                                       | Documentation                                                 |
|-----------------|---------|-------------------------------------------------------------------------------------------------------|----------------------------------------------------------------|
| **Alpine.js**   | `x-`    | `<div x-data="{ open: false }" x-show="open"></div>`                                                 | [Documentation Alpine.js](https://alpinejs.dev/)               |
| **Vue.js**      | `v-`    | `<input v-model="message" v-if="show">`                                                              | [Documentation Vue.js](https://vuejs.org/guide/)               |
| **Stencil**     | `@`     | `<my-component `@onClick`="handleClick"></my-component>`                                               | [Documentation Stencil](https://stenciljs.com/docs/)           |
| **Lit**         | `@`     | `<button `@click`="${this.handleClick}">Click me</button>`                                             | [Documentation Lit](https://lit.dev/docs/)                    |

Commits
-------

7bd2854 Improve __toString performance
cb7809f Add str_contains in nested method
9f7c9c8 Performance
3452436 fabbot
1ed1db7 [TwigComponent] Ignore "nested" for Alpine & Vue attributes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants