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] Add the live.component service tag #1693

Open
wants to merge 3 commits into
base: 2.x
Choose a base branch
from

Conversation

jakubtobiasz
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
Issues n/a
License MIT

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 a live.component service tag, which will be translated into twig.component tag 🕺🏻.

Time spent on this feature has been sponsored by Commerce Weavers ♥️.

@jakubtobiasz jakubtobiasz force-pushed the live_component_tag branch 2 times, most recently from 42088f3 to 5898f3c Compare April 8, 2024 11:25
@jakubtobiasz jakubtobiasz marked this pull request as ready for review April 8, 2024 13:20
@carsonbot carsonbot added the Status: Needs Review Needs to be reviewed label Apr 8, 2024
@smnandre
Copy link
Collaborator

smnandre commented Apr 8, 2024

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

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 ?

Copy link
Contributor Author

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this absolutely necessary ?

Copy link
Contributor Author

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:

  1. it makes testing compiler passes, extension and other things way easier
  2. 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.

Copy link
Member

@kbond kbond Apr 9, 2024

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!

@kbond
Copy link
Member

kbond commented Apr 16, 2024

@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)?

  • The twig template is straight forward as we can use the standard bundle overrides.
  • We're not sure about component class overrides. One idea I floated was if you have a twig component that extends a 3rd party one, we merge the config (with the user defined config taking presidence).

@jakubtobiasz
Copy link
Contributor Author

@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 😅.

@kbond
Copy link
Member

kbond commented Apr 24, 2024

what config do you mean 🤔?

The tag/attribute config:

// in bundle
#[AsLiveComponent(...)]
BaseButtonComponent {}

// in app
#[AsLiveComponent(...)]
ButtonComponent extends BaseButtonComponent {}

We'd merge the ButtonComponent's tag attributes with BaseButtonComponent.

@jakubtobiasz
Copy link
Contributor Author

jakubtobiasz commented Apr 27, 2024

@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 .xml file and the live.component attribute. If someone wants to change anything, they should override the service with using the .yaml (or any other format) file. Alternatively, they can extend the original service and create a new one basing on it. Also the component can be decorated.

@smnandre
Copy link
Collaborator

But props or methods do need Attributes right ?

@kbond
Copy link
Member

kbond commented Apr 27, 2024

keep in mind that the official recommendation is to not using attributes/annotations inside the bundles.

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).

@jakubtobiasz
Copy link
Contributor Author

But props or methods do need Attributes right ?
Yup, but it's a class-level metadata, not related to the DI, which is the main reason why attributes are not recommended for bundles.

If you extend a class, and you want to change anything in existing attributes, you have to declare the attribute on your own.

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).

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 getTemplate(): string method (similarly like extending forms is solved).

Comment on lines +43 to +45
'url_reference_type' => $tag['url_reference_type'] ?? UrlGeneratorInterface::ABSOLUTE_PATH,
]);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Review Needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants