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

[LiveComponent] remove final #1352

Closed
wants to merge 1 commit into from
Closed

[LiveComponent] remove final #1352

wants to merge 1 commit into from

Conversation

jannes-io
Copy link
Contributor

Q A
Bug fix? no
New feature? no
Issues Fix #1351
License MIT

Remove final from AsLiveComponent so implementors can create abstractions. See #1351 for use-case examples.

@smnandre
Copy link
Collaborator

I'm not sure we should remove the "final", as the "trend" in symfony/symfony is more to add "final" where it's possible.

If you search "final" in the main symfony repository issues, you'll find several similar suggestions. In the answers you'll often find Symfony core team members giving the reasons why.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 22, 2023

It all depends, there's no dogma on the topic.
But here, removing final won't help: using it will still require a dedicated call to registerAttributeForAutoconfiguration to make any child class work.
I would even go one step further and recommend that AsLiveComponent shouldn't extend AsTwigComponent (because the inheritance doesn't mean anything from an abstraction pov).

@jannes-io
Copy link
Contributor Author

In the answers you'll often find Symfony core team members giving the reasons why.

If there's a solid reason for something to be marked final, then I agree. If it's just because "the trend is x", then that's not good enough...

using it will still require a dedicated call to registerAttributeForAutoconfiguration to make any child class work.

Can't the abstract class declare the required tags using attributes as well?

@smnandre
Copy link
Collaborator

If I mentionned the multiple similar topic it’s because I dont want to explain « my » vision, and would not do it well haha

If i used the word trend it’s to say « and its more going in the opposite direction right now, so don’t give too much hope in a change in the near future ».

@jannes-io
Copy link
Contributor Author

Ok gents, thanks for the quick responses! I’ll go a different route and register the custom attribute for auto wiring in my bundle the same way live component is auto registered. I thought perhaps because Live component extends TwigComponent that this was the way to go but it’s not a big deal to register it manually 😊

Thanks again.

@jannes-io jannes-io closed this Dec 22, 2023
@smnandre
Copy link
Collaborator

Not sure if that would work.. (as you do not a matching template at first) but you can look at the PreRenderEvent and see if you can change the template there ?

https://symfony.com/bundles/ux-twig-component/current/index.html#prerenderevent

@jannes-io
Copy link
Contributor Author

Not sure if that would work.. (as you do not a matching template at first) but you can look at the PreRenderEvent and see if you can change the template there ?

https://symfony.com/bundles/ux-twig-component/current/index.html#prerenderevent

Thanks for the pointer @smnandre !

@smnandre
Copy link
Collaborator

Another "hacky but probably working" solution would be to create a "AliasLoader" and pass it to Twig, that would resolve some templates to the one you'd want.

I would not advise you to manually register your components as things can still change a bit on this part of the component :|

@smnandre
Copy link
Collaborator

It all depends, there's no dogma on the topic. But here, removing final won't help: using it will still require a dedicated call to registerAttributeForAutoconfiguration to make any child class work. I would even go one step further and recommend that AsLiveComponent shouldn't extend AsTwigComponent (because the inheritance doesn't mean anything from an abstraction pov).

I'm sorry i would not have answered if i saw that you already answered... i just got a notification and saw your comment oO It did not appear before.. even when i re-re-answered... Did you use some dark github magic ? 🧙‍♂️

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 this pull request may close these issues.

[LiveComponent] make AsLiveComponent non-final
3 participants