symfony / ux

Symfony UX initiative: a JavaScript ecosystem for Symfony
https://ux.symfony.com/
MIT License
858 stars 315 forks source link

[LiveComponent] Multiple URL query params in one LiveProp DTO #2142

Open Nayte91 opened 2 months ago

Nayte91 commented 2 months ago

Hello,

Part of the discussion about faceted search menu and results page combo (challenge 1), I'm searching to map different url parameters into one object.

In a symfony controller, you can welcome several query params in a single DTO with the MapQueryString attribute.

I wonder if we can, also in live components, group several query params into one property class?

In a live component, currently, when you have multiple query parameters to welcome, you must declare them one by one; then eventually, in a method, regroup them into an array or a DTO to do some logic. It can be cumbersome if you have a lot of potential query parameters. Plus, and I feel like it's more important, aligning behavior of component controllers (like a LC), with regular controllers is very important as it drastically lowers the mind burden and learning curve.

Context: you got a FacetedSearchMenu component, with multiple parameters that you want to reflect on url, and you group those in a FacetFilter DTO to pass to repository or whatever.

If I may help in any way, Best regards,

Nayte91 commented 1 month ago

Possibilities:

smnandre commented 1 month ago

I'm having doubt here... are DTO as LiveProp supposed to work already, even with URL ....

https://symfony.com/bundles/ux-live-component/current/index.html#supported-data-types

Nayte91 commented 1 month ago

Interesting point, did you achieve any minimal working example? Because I try since 1hour, and I always throw errors:

An exception has been thrown during the rendering of a template ("Warning: Undefined property: App\Component\ArticleResults::$name").

class ArticleResults
{
    use DefaultActionTrait;
    use ComponentToolsTrait;

//    #[LiveProp(url: true)]
//    public ?string $name = null;
//    #[LiveProp(url: true)]
//    public ?string $status= null;
//    #[LiveProp(writable: true, url: true)]
//    public ?int $page = 1;

    #[LiveProp(writable: true, url: true)]
    public ?Filter $filter = null;

    private ItemsPage $articlesPage;

    public functiondoStuff(): foo {...}
}

class Filter
{
    public function __construct(
        public ?int $page = 1,
        public ?string $name = null,
        public ?string $status = null,
    ) {}
}

I may miss something goofy here, but for me and based also on the DTO part of the doc, I can't make it work 😢

smnandre commented 1 month ago

The error message you quote may indicate you try to mount "name" directly on ArticleResults in your template, no ?

smnandre commented 1 month ago

https://github.com/user-attachments/assets/75357178-bc62-4d18-945f-2ac18c29e25e

smnandre commented 1 month ago

(sorry for the support page ... i was working on something else 😅 )

smnandre commented 1 month ago
<?php

namespace App\Twig;

use Symfony\UX\LiveComponent\Attribute\AsLiveComponent;
use Symfony\UX\LiveComponent\Attribute\LiveProp;
use Symfony\UX\LiveComponent\DefaultActionTrait;

#[AsLiveComponent]
final class Search
{
    use DefaultActionTrait;

    #[LiveProp(writable: ['search', 'size', 'type'], fieldName: 's', url: true)]
    public SearchFilters $filters;

    public function mount(): void
    {
        $this->filters ??= new SearchFilters();
    }

    public function getResults(): array
    {
        $results = [
            ['name' => 'Gorilla', 'size' => 'lg',  'type' => 'animal'],
            ['name' => 'Banana', 'size' => 'md',  'type' => 'fruit'],
            ['name' => 'Apple', 'size' => 'sm',  'type' => 'fruit'],
            ['name' => 'Lion', 'size' => 'lg',  'type' => 'animal'],
            ['name' => 'Strawberry', 'size' => 'sm',  'type' => 'fruit'],
            ['name' => 'Elephant', 'size' => 'lg',  'type' => 'animal'],
            ['name' => 'Orange', 'size' => 'md',  'type' => 'fruit'],
            ['name' => 'Tiger', 'size' => 'lg',  'type' => 'animal'],
            ['name' => 'Pineapple', 'size' => 'md',  'type' => 'fruit'],
            ['name' => 'Zebra', 'size' => 'lg',  'type' => 'animal'],
        ];

        $results = array_filter($results, function($result) {
            return $this->filters->search === '' || str_contains($result['name'], $this->filters->search);
        });

        $results = array_filter($results, function($result) {
            return $this->filters->size === '' || $this->filters->size === $result['size'];
        });

        $results = array_filter($results, function($result) {
            return $this->filters->type === '' || $this->filters->type === $result['type'];
        });

        return $results;
    }
}
<?php

namespace App\Twig;

use Symfony\UX\LiveComponent\Attribute\AsLiveComponent;
use Symfony\UX\LiveComponent\DefaultActionTrait;

final class SearchFilters
{
    public string $search = '';

    public string $type = '';

    public string $size = '';
}
<div{{ attributes }}>

    <h2>dump(this.filters)</h2>
    {{ dump(this.filters) }}

    <hr />

    <h2>Filters</h2>
    <div style="display: grid; grid-template-columns: 1fr 1fr 1fr;">

        <div>
            <label>Search</label>
            <input type="search" class="form-control" data-model="s[search]" />
        </div>

        <div>
            <label>Size</label>
            <select class="form-control" data-model="s[size]">
                <option value="">Size</option>
                <option value="sm">Small</option>
                <option value="md">Medium</option>
                <option value="lg">Large</option>
            </select>
        </div>

        <div>
            <label>Type</label>
            <label>
                <input type="radio" class="form-check" data-model="s[type]" value="animal" />
                Animal
            </label>
            <label>
                <input type="radio" class="form-check-input" data-model="s[type]" value="fruit" />
                Fruit
            </label>
        </div>
    </div>

    <hr />

    <div>
        <h2>Results</h2>

        {% for result in this.results %}
            <div id="{{ result.name }}" style="padding: 1rem 0; border-bottom: 2px dotted #eee;">
                <div class="d-flex justify-content-between">
                    <h5>{{ result.name }}</h5>
                    <span>{{ result.size }}</span>
                    <span>{{ result.type }}</span>
                </div>
            </div>
        {% endfor %}
    </div>

</div>
smnandre commented 1 month ago

(please do not look too much at the code quality 😆 )

Nayte91 commented 1 month ago

Thank you very much for taking the time to create example, I made some tries and here's my thoughts:

class FacetMenu
{
    use DefaultActionTrait;

    #[LiveProp(writable: ['page'], onUpdated: 'emitChange', url: true)]
    public Filter $filter;

    public function mount() { $this->filter ??= new Filter; }

    public function emitChange(): void
    {
        dd('hello');
        $this->emit('facetSetted', ['filter' => $this->filter]); // for the next point
    }
}
class EventResults
{
    use DefaultActionTrait;

    #[LiveProp]
    public Filter $filter;

    #[LiveListener('facetSetted')]
    public function reload(#[LiveArg] array $filter): void 
    {
        dd($filter);
    }
}

All those constraints make the move quite complicated; I feel like using a DTO is way less intuitive, but I frankly don't know how to handle those issues 😶

smnandre commented 1 month ago

You're very right on all these points.

So, now... would you want to help improving one ? And see where that leads ?

I can help, pin in the direction if you need, and probably help you figuring "what's not gonna stay that way" and "what is needed because this or that features"...

What do you think ?

Nayte91 commented 1 month ago

I definitely accept the challenge of open source!

  1. Adding a #[Mount] attribute: copy/pasting another attribute so trivia I guess (famous last words)
  2. Making #[LiveProp(writable: true)] works on a DTO seems mandatory and hard to counter argue?
  3. Making #[LiveProp(onChange: 'foo')] works on a DTO seems mandatory also?
  4. Having a mount() method to help a DTO to be created is sad, but I guess it's quite tricky to avoid,
  5. [LiveArg] that could accept an object and auto normalize it, as a regular Symfony controller, is I think, a crossroads. My feeling as a newbie is that the regular SF controllers are very conveniants and intuitives, and the different behavior of LC is suboptimal and creates mind burden. So instead of fixing "LiveArg" I feel like LC should have a 'ControlerToolkit' addon, maybe a trait, or a abstract class, but tweaked for LCs (I know that you can extends AbstractController for a LC, but I guess a random method can't accept params normalization like controllers can), that will help to play with URLs and serlializations, because at the end the day, my whole problems/suggestions are basically "my LC is a little controller that need controller's power, please halp".

smnandre commented 1 month ago

Would you agree to take some time to discuss them (slack / meet / anything / ?) when you have the time ?

Just quickly

  1. mount() method is called if exist, so not sure there really is a need here.

  2. I'm certain this will be more complicated than it seems right now... but yeah the DX is not optimal currently. Let's just keep in mind that LiveProp is a "solution" and maybe there is something else to consider.... like "what is a liveprop" and "do we need something else ?

  3. Yes, nothing to think of too much for now, as it entirely depends on the 2

  4. Not sure to understand here? You could instanciate it in the constructor, or before calling the component the first time... .but if you have only an empty class string ... is it really a liveprop you need, or a logger ?

  5. This is were i'm a bit :-/ A component should only react to a small (limited) set of methods (the actions), value changes, etc... so i would almost say the opposite, what would they use a request for ? a firewall ? a flash message. So here we see everyone has its own definition of what is a component, a statefull one, or not :) And i'm really not sure some implementation details we have right now are made for all these different visions. But all of them are legit and we need to clarify how we want to handle them and lower the frustrations.


At the same time of all your very relevant ideas and feedback... I also started to split more clearly Live and Twig component.. so i huess the work on Attributes, Metadata, Events is a must-have to clarify some behaviours, and free some place for new features :)

Nayte91 commented 1 month ago

I would be pleased to speak about this on slack for example; this week my planning is a wreck, I just write issues when I have time, but next week for sure.

Let me advocate for #[Mount] one last time :sweat_smile: as it is very small:

  1. #[PreMount] & #[PostMount] already exist,
  2. It helps code expressivity, as you may want to save one LOC and call your method mount(), but you can also want it expressive, call #[Mount] and create your method public function setPropFromRepository() (ok that's a very bad name),
  3. This SF pattern is common and exists everywhere, for example event listeners let you name your function 'onFoo()', or let you put an attribute with #[AsEventListener(event: 'foo')] and call your method however you want,
  4. Note that for completion, I don't know if you can currently create a postMount() function without attribute and make it behave like it has #[PostMount] :smile:
  5. If it's done, I feel like we can create a small new 'LC events' of documentation, with those 3 and the tricks, make refs from the main page, and begin to slim down the doc 🥳
Nayte91 commented 1 month ago

Not sure to understand here? You could instanciate it in the constructor, or before calling the component the first time... .but if you have only an empty class string ... is it really a liveprop you need, or a logger ?

Point 4, about DTO initialization: I feel like we should left this one behind, unless someone comes with a veeery clever trick.

Maybe the most optimal version will be in the language, being able to declare properties like public SearchFilter $filter = new SearchFilter() I don't know if it's packed in 8.4 or will ever happen.

Anyway, it can be done also in constructor, I didn't think about it, nice trick 👌

Nayte91 commented 1 month ago
  1. This is were i'm a bit :-/ A component should only react to a small (limited) set of methods (the actions), value changes, etc...

Points 2 and 3 about LiveProp, needs to be clear about 5, 'general design' considerations. We agree that DX is not optimal, that TC and LC need more separation, and new features I am requesting (except Mount that is a bit stand alone) must integrate well with general design.

I will take time to read your links about the split, but let me give some food here:

Based on those highly debatable points, I feel like in an ideal world:

That's some work and impossible wishes, but that's my statement for now :laughing:

Nayte91 commented 1 month ago

so i would almost say the opposite, what would they use a request for ? a firewall ? a flash message.

To answer about request, some reasons as I mix good & bad ideas:

You're right for this part, I feel like none of my arguments are killer ones, and a good shaped LC can get rid of any Request need, on classical use cases (if some nerds want to inject it by constructor, so be it).

smnandre commented 1 month ago

To be honest, I think we share the same vision on the vast majority of things here... i may have not expressed my personal ideas here with enough talent 😅

The remaining differences are purely philosophical and won't be a problem here :)