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

[TwigComponent] Remove @internal from ComponentFactory #1842

Open
onEXHovia opened this issue May 12, 2024 · 7 comments
Open

[TwigComponent] Remove @internal from ComponentFactory #1842

onEXHovia opened this issue May 12, 2024 · 7 comments

Comments

@onEXHovia
Copy link
Contributor

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

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

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

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

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

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

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

kbond commented May 15, 2024

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.

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

No branches or pull requests

4 participants