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] Add the live.component
service tag
#1693
base: 2.x
Are you sure you want to change the base?
Conversation
42088f3
to
5898f3c
Compare
5898f3c
to
6d7051e
Compare
4f50958
to
274dfb8
Compare
Hey! I think this will require a bit of time to study, cause it raises some questions we already had hard times on regarding what we allow to be configured / overwritten by bundles or not, and the order of registration :) But i like the idea :) |
@@ -128,6 +128,8 @@ jobs: | |||
dependency-versions: ${{ matrix.dependency-version }} | |||
|
|||
- name: ${{ matrix.component }} Tests | |||
env: | |||
SYMFONY_DEPRECATIONS_HELPER: 'max[total]=999999' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you create a separate PR for this change ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing, the question is whether we want (or must) do it this way. It's a little hack, but on the other hand, the question is whether we need this information in the CI.
@@ -37,6 +37,7 @@ | |||
"doctrine/doctrine-bundle": "^2.4.3", | |||
"doctrine/orm": "^2.9.4", | |||
"doctrine/persistence": "^2.5.2|^3.0", | |||
"matthiasnoback/symfony-dependency-injection-test": "^5.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this absolutely necessary ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course it isn't. But I decided to add it as:
- it makes testing compiler passes, extension and other things way easier
- we're using it in Sylius, and it's a trustworthy dependency
If you want, I can remove it and test the compiler pass other way, but if we plan to create more tests covering extensions/compiler passes, it might be cool to leave it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this package - as config/compiler passes get more complex it's super useful - let's keep it!
@jakubtobiasz, for probably v2.18 or v2.19, we're going to really nail down the best practice for providing twig/live components in bundles. Do you have any suggestions on this (aside from this PR)?
|
@kbond what config do you mean 🤔? I didn't take a look at extendibility (yet), but I bet (as usually) decorating/extending the class + optionally override the template. In Sylius we have to care only about the class, because it's our "source of data". In terms of templates, we're using our hooks system, so we don't need to worry about templates 😅. |
The tag/attribute config: // in bundle
#[AsLiveComponent(...)]
BaseButtonComponent {}
// in app
#[AsLiveComponent(...)]
ButtonComponent extends BaseButtonComponent {} We'd merge the |
@kbond keep in mind that the official recommendation is to not using attributes/annotations inside the bundles. So, the configuration on the bundle-level should be done with using |
But props or methods do need Attributes right ? |
Yeah, I have been thinking to make an exception for twig/live components just because I think the tag config is a lot (but maybe that's just me). |
If you extend a class, and you want to change anything in existing attributes, you have to declare the attribute on your own.
We might think of moving some configuration from the tag level to a getter inside the component. So, you can define a template on a tag level or inside the |
'url_reference_type' => $tag['url_reference_type'] ?? UrlGeneratorInterface::ABSOLUTE_PATH, | ||
]); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'url_reference_type' => $tag['url_reference_type'] ?? UrlGeneratorInterface::ABSOLUTE_PATH, | |
]); | |
} | |
'url_reference_type' => $tag['url_reference_type'] ?? UrlGeneratorInterface::ABSOLUTE_PATH, | |
]); | |
$liveComponentService->addTag('controller.service_arguments'); | |
} |
While implementing this pass in Sylius, I've discovered I missed adding the controller.service_arguments
tag. I'll update the PR soon.
Currently we've been forced to use
twig.component
tag to configure live components manually. It required a lot of boilerplate code and sometimes could cause some issues when a new attribute has appeared (we faced with it once or twice in Sylius). So, the idea is to add alive.component
service tag, which will be translated intotwig.component
tag 🕺🏻.