symfony / ux

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

[TwigComponent] Remove @internal from `ComponentFactory` #1842

Open onEXHovia opened 5 months ago

onEXHovia commented 5 months ago

Related #707

It should be possible to create a component manually. Usually it's a few steps:

  1. Use factory for create component
  2. Deserializing request on component via serializer or use libliary for mapping
  3. Render component

The described workflow occurs in LiveComponent, the same functional should be for TwigComponent

smnandre commented 5 months ago

I’m not sure to follow. What do you call « manually » ? And the worflow you describe (request / serialization / render) is already possible with … a controller ?

Maybe could you illustrate what you’re trying/would like to do ?

onEXHovia commented 5 months ago

An example of the controller I'm talking about, I'm not sure it works. ComponentFactory and ComponentRenderer @internal now.

use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Routing\Annotation\Route;
use Symfony\Component\Serializer\Normalizer\AbstractNormalizer;
use Symfony\Component\Serializer\SerializerInterface;
use Symfony\UX\TwigComponent\ComponentFactory;
use Symfony\UX\TwigComponent\ComponentRenderer;

final class ComponentManualController
{
    public function __construct(
        private readonly ComponentFactory $componentFactory,
        private readonly ComponentRenderer $renderer,
        private readonly SerializerInterface $serializer,
    ) {
    }

    #[Route('/_component', name: 'component_example', methods: ['GET'])]
    public function __invoke(Request $request): Response
    {
        $name = 'example';
        $component = $this->componentFactory->get($name);

        // or use another mapper
        $this->serializer->deserialize($request->query->all(), $component::class, null, [
            AbstractNormalizer::OBJECT_TO_POPULATE => $component,
        ]);

        $metadata = $this->componentFactory->metadataFor($name);
        $mount = $this->componentFactory->mountFromObject($component, [], $metadata);

        return new Response($this->renderer->render($mount));
    }
}
smnandre commented 5 months ago

I'm sorry but ... why do you need any component here ? The factory is used to handle name -> class, the "mounting" and the template... all thing you could do directly from your controller .. or am i missing something ?

Maybe with a concrete illustration of what would be the "component_example" ?

onEXHovia commented 5 months ago

@kbond @weaverryan

Very often there is a need to update a component on page via ajax or turbo stream. With the current API it is possible like this:

use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Routing\Annotation\Route;
use Symfony\UX\TwigComponent\ComponentRendererInterface;

final class ExampleController
{
    public function __construct(
        private readonly ComponentRendererInterface $renderer
    ) {
    }

    #[Route('/_component/{foo}', name: 'component_example', methods: ['GET'])]
    public function __invoke(string $foo): Response
    {
        return new Response($this->renderer->createAndRender('name_component', [
            'foo' => $foo,
        ]));
    }
}

As components grow, this becomes a problem. But it is not possible to create a "universal" controller since the api is marked internal. Sample code above, you need create component, mapped request and render him.

TwigComponent and LiveComponent these are two different libraries, but LiveComponent use internal api TwigComponent which of course shouldn't happen. The more flexible API TwigComponent, the easier it will be LiveComponent and other soluitions who use TwigComponent.

Maybe should add DataMapperInterface in TwigComponent how it's done in Symfony form component this will help simplify LiveComponentHydrator‎ and stop using internal API.

smnandre commented 5 months ago

ComponentFactory and ComponentRenderer @internal now.

ComponentRendererInterface is not internal, and so is an extension point you can use as you wish.

which of course shouldn't happen.

It's an opinion that i can ear, but do not share :)

The more flexible API TwigComponent, the easier it will be LiveComponent

LiveComponent have no problem with TwigComponent... it's probably more the opposite right now :)

and other soluitions who use TwigComponent.

TwigComponent are meant to be .... Twig component. So the entire worflow is currently designed, coded & optimized to be used from a template, with no knowledge of request, controllers, etc...

how it's done in Symfony form component

... whereas the main reason of the Form component is to map request data to the view and backward.

The main reason to not add aditional extension point is to keep the component maintanable, and ensure it will work in the way it is supposed to (by calling render directly you bypass some event for instance).

That's why i would personally not be in favor to expose any other contract than the existing ones.

onEXHovia commented 5 months ago

ComponentFactory and ComponentRenderer @internal now.

ComponentRendererInterface is not internal, and so is an extension point you can use as you wish.

Sorry, If this were enough, I wouldn't open the request, right?

which of course shouldn't happen.

It's an opinion that i can ear, but do not share :)

So, if someone wants to make a package similar to LiveComponent, should he use internal api TwigComponent?

how it's done in Symfony form component

... whereas the main reason of the Form component is to map request data to the view and backward.

This has nothing to do with what I wrote. Signature methods DataMapperInterface interface may be other and adapted to the components. PropertyAccessor is used throughout the codebase one way or another for mapping values properties, anyway it's just a suggestion.

ok, I understand your position, but also interested in the opinions of other maintainers.

kbond commented 5 months ago

I'll review this more thoroughly when I have some time but just wanted to make a note about our use of @internal (or at least why it was added to these components). Basically, during the heavy development stage of a package, I basically mark everything as @internal. This let's us move unhindered during heavy development - even post 1.0. A good example is ux-icons - literally everything is internal. We have some plans that will likely refactor/change interfaces but we can do this w/o BC breaks (as long as we keep the end-usage free of BC breaks).

As a package matures and a valid use-case comes up to use a service/object that's marked internal in user-land, I have no issue removing. I feel twig components is at this stage now.