Twig/LiveComponent Attributes #106

kbond · 2021-06-21T14:55:43Z

Q	A
Bug fix?	no
New feature?	no
Tickets	-
License	MIT

Based on discussions with @weaverryan, the consensus from the core team seems to be to require PHP8+ and use attributes for defining components instead of the interface. The updated readme shows how this would now work.

Assuming this is the direction we want to go, I will update the LiveComponent package.

Todo:

Update LiveComponent
Fix duplicated LiveProp issue on parent classes

src/TwigComponent/src/Attribute/TwigComponent.php

weaverryan

I like this approach!

src/TwigComponent/src/Attribute/TwigComponent.php

wouterj · 2021-06-21T20:31:13Z

I quickly discussed this on Slack already, but I think we should use a DI tag + registerAttributeForAutoconfiguration() for configuring components (and name + template). That seems to be the standard for all other Symfony attributes and it adds a nice extra: It means we have support for PHP <8.0 and Symfony <5.3 if a user just adds the tag to the component services.

It does require some rewriting of the renderer though, as it currently uses the attribute at runtime.

Is there anyone that can share their experiences with attributes on which approach is the best (based on performance or other thing that we didn't come up with)?

dbrekelmans

You were just a bit ahead of me.. I hadn't seen this PR and was working on an almost identical implementation ;) Looks good so far!

src/TwigComponent/src/DependencyInjection/Compiler/TwigComponentPass.php

kbond · 2021-06-21T21:27:28Z

I now use registerAttributeForAutoconfiguration in lieu of the compiler pass. Twig components can also be manually configured by adding a twig.component tag with a key parameter for the name.

The failing test is related to components with the same name. I no longer ensure they are unique. Right now, the last one registered wins. Thoughts? Should I add a compiler pass to ensure they are unique? (I believe a compiler pass is the only way)

Also, I now require Symfony 5.3+ so I can use registerAttributeForAutoconfiguration but could allow older version if desired. On older versions, components could only be registered manually.

src/TwigComponent/README.md

weaverryan · 2021-06-22T11:28:04Z

The failing test is related to components with the same name. I no longer ensure they are unique. Right now, the last one registered wins. Thoughts? Should I add a compiler pass to ensure they are unique? (I believe a compiler pass is the only way)

I vote for last one wins - like service id's

Also, I now require Symfony 5.3+ so I can use registerAttributeForAutoconfiguration but could allow older version if desired. On older versions, components could only be registered manually.

If it's just a matter of doing a method_exists() before calling that method, then I'm +1 for doing that and at least allowing 5.2 and lower users to use this library with manual tagging.

src/TwigComponent/src/ComponentRenderer.php

kbond · 2021-06-22T16:45:56Z

Do we want the AsTwigComponent to be completely optional as @dbrekelmans suggests above? Allow both the component name and template to be set manually via the twig.component tag? How should I map the component to a template for the ComponentRenderer? Build a component class => template map?

kbond · 2021-06-22T17:41:26Z

I vote for last one wins - like service id's

I went with this.

If it's just a matter of doing a method_exists() before calling that method, then I'm +1 for doing that and at least allowing 5.2 and lower users to use this library with manual tagging.

Manual tagging/4.4+ is now supported (I haven't updated the gh action to prove this). PHP8 and the AsLiveComponent is still required. Manual tagging is a little annoying as your key tag attribute must match your AsLiveComponent name property.

weaverryan · 2021-06-23T13:15:04Z

Do we want the AsTwigComponent to be completely optional as @dbrekelmans suggests above?

Maybe? Right now we only have name and template options... but there is already the question of:

How should I map the component to a template for the ComponentRenderer? Build a component class => template map?

And, what if we have more options in the future? I can envision that, if we decide to put it all on the tag, then we would, in a compiler pass, push that tag data into some sort of "component metadata service" where we would probably have a method like getMetadata($class) that would return an object that's very similar to the AsLiveComponent. But it would be tricky to make this service lazy (we would probably inject ALL of the tag data from ALL of the components into its constructor, so instantiating it would put that all into memory).

So I think we should NOT do this. We should follow the API Platform model (or the Doctrine entity model) where the metadata is pulled lazily at runtime when needed. The only stuff in the tag is the stuff that is useful to know without needing to instantiate the objects (e.g. with commands, it's nice to know the name of each command without instantiating it, and the same with Twig components).

If we were to make the PHP8 attribute optional, I think it makes sense to do so with an alternate configuration format - e.g. a PHP config format (like routing config or validation config).

weaverryan · 2021-06-23T13:16:01Z

Manual tagging is a little annoying as your key tag attribute must match your AsLiveComponent name property.

What happens if it doesn't? It seems like an odd situation, but I would imagine that the tag name would "win" over the one in the attribute.

kbond · 2021-06-23T13:36:04Z

So I think we should NOT do this

I agree.

What happens if it doesn't?

For twig components, I don't believe it will be a problem as we never, at runtime, get the component name from the class. For live components, as of right now, the url is generated with the name the component class gives. The subscriber would look for the component based on the DI tag so there could be a mismatch.

I could enforce this in the compiler pass.

dbrekelmans · 2021-06-23T13:40:34Z

Fair points @weaverryan, I agree.

kbond · 2021-06-23T14:00:23Z

I could enforce this in the compiler pass.

Alternatively, and I think I prefer this, I can remove the requirement to set a tag key and configure the service locator in the compiler pass. This would remove any possibility of a mismatch when manually tagging.

kbond · 2021-06-23T15:23:27Z

I think the TwigComponent refactor is complete and ready for review. I added a GH action job to run on php8 with low deps to prove it's compatible with 4.4.

Alternatively, and I think I prefer this, I can remove the requirement to set a tag key and configure the service locator in the compiler pass. This would remove any possibility of a mismatch when manually tagging.

I went with this. No possibility for a mismatch. The "name" is now only pulled from the attribute.

weaverryan

Minor comments - but this looks really solid! Let's roll this into LiveComponents 👍

src/TwigComponent/README.md

src/TwigComponent/src/ComponentRenderer.php

src/TwigComponent/src/DependencyInjection/Compiler/TwigComponentPass.php

src/TwigComponent/src/DependencyInjection/TwigComponentExtension.php

kbond · 2021-06-27T14:01:40Z

I have switched LiveComponents to PHP8 attributes (only) so this is ready for another round of reviews (ping @dbrekelmans, @weaverryan).

~~Not sure why the coding-style-js job is failing.. Looks to be caused by the live component readme.~~ fixed

ker0x · 2021-06-27T21:58:09Z

src/LiveComponent/src/Attribute/LivePropContext.php

+    private LiveProp $liveProp;
+    private \ReflectionProperty $reflectionProperty;
+
+    public function __construct(LiveProp $liveProp, \ReflectionProperty $reflectionProperty)


As the package now requires PHP 8, wouldn't it be interesting to use constructor property promotion here and in other classes where it can be implemented?

Absolutely, I didn't use these PHP 8 features as it breaks the php-cs-fixer job for the mono repo. I'm not sure how we want to handle this.

weaverryan

Minor notes - this is looking great!!!

src/LiveComponent/composer.json

src/LiveComponent/src/Attribute/AsLiveComponent.php

src/LiveComponent/src/Attribute/LivePropContext.php

src/LiveComponent/src/DependencyInjection/LiveComponentExtension.php

dbrekelmans

Nice work @kbond!

src/LiveComponent/README.md

src/LiveComponent/src/Attribute/AsLiveComponent.php

src/LiveComponent/src/EventListener/LiveComponentSubscriber.php

src/LiveComponent/tests/Fixture/Kernel.php

src/TwigComponent/tests/Fixture/Kernel.php

weaverryan

👍

weaverryan · 2021-07-01T12:50:14Z

Thank you Kevin for this huge work!!

This PR was squashed before being merged into the main branch. Discussion ---------- Twig/LiveComponent Attributes | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | Tickets | - | License | MIT Based on discussions with `@weaverryan`, the consensus from the core team seems to be to require PHP8+ and use attributes for defining components instead of the interface. The updated [readme](https://github.com/symfony/ux/blob/a80d8df64a41860d2fec407dfecac36ce34809a1/src/TwigComponent/README.md) shows how this would now work. Assuming this is the direction we want to go, I will update the `LiveComponent` package. Todo: - [x] Update `LiveComponent` - [x] Fix duplicated `LiveProp` [issue](symfony/ux#106 (comment)) on parent classes Commits ------- 46777c3 Twig/LiveComponent Attributes

This PR was squashed before being merged into the main branch. Discussion ---------- Update for twig/live component attribute refactor This is a companion PR for symfony/ux#106. Commits ------- beb13ee Update for twig/live component attribute refactor

This PR was merged into the main branch. Discussion ---------- Adding CHANGELOG for twig/live component | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | Tickets | none | License | MIT Follow up of #106 to help people track what's changing. Commits ------- ddac904 Adding CHANGELOG for twig/live component

This PR was merged into the main branch. Discussion ---------- [bug] remove duplicated doctrine/annotations entry | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | Tickets | n/a | License | MIT Looks like 2 different PR's added this. (#111 and #106) Commits ------- 5b9ea2d [bug] remove duplicated doctrine/annotations entry

This PR was squashed before being merged into the main branch. Discussion ---------- Twig/LiveComponent Attributes | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | Tickets | - | License | MIT Based on discussions with `@weaverryan`, the consensus from the core team seems to be to require PHP8+ and use attributes for defining components instead of the interface. The updated [readme](https://github.com/symfony/ux/blob/a80d8df64a41860d2fec407dfecac36ce34809a1/src/TwigComponent/README.md) shows how this would now work. Assuming this is the direction we want to go, I will update the `LiveComponent` package. Todo: - [x] Update `LiveComponent` - [x] Fix duplicated `LiveProp` [issue](symfony/ux#106 (comment)) on parent classes Commits ------- 46777c3 Twig/LiveComponent Attributes

This PR was merged into the main branch. Discussion ---------- [bug] remove duplicated doctrine/annotations entry | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | Tickets | n/a | License | MIT Looks like 2 different PR's added this. (symfony/ux#111 and symfony/ux#106) Commits ------- 5b9ea2d [bug] remove duplicated doctrine/annotations entry

kbond force-pushed the attributes branch from 1b36e51 to a80d8df Compare June 21, 2021 15:00

kbond commented Jun 21, 2021

View reviewed changes

src/TwigComponent/src/Attribute/TwigComponent.php Outdated Show resolved Hide resolved

kbond commented Jun 21, 2021

View reviewed changes

src/TwigComponent/src/Attribute/TwigComponent.php Outdated Show resolved Hide resolved

weaverryan reviewed Jun 21, 2021

View reviewed changes

src/TwigComponent/src/Attribute/TwigComponent.php Outdated Show resolved Hide resolved

src/TwigComponent/src/Attribute/TwigComponent.php Outdated Show resolved Hide resolved

src/TwigComponent/src/Attribute/TwigComponent.php Outdated Show resolved Hide resolved

kbond mentioned this pull request Jun 21, 2021

[LiveComponent] Meta issue for todos, changes & bugs #102

Open

dbrekelmans reviewed Jun 21, 2021

View reviewed changes

src/TwigComponent/src/DependencyInjection/Compiler/TwigComponentPass.php Outdated Show resolved Hide resolved

src/TwigComponent/src/DependencyInjection/Compiler/TwigComponentPass.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Jun 22, 2021

View reviewed changes

src/TwigComponent/README.md Outdated Show resolved Hide resolved

dbrekelmans reviewed Jun 22, 2021

View reviewed changes

src/TwigComponent/src/ComponentRenderer.php Outdated Show resolved Hide resolved

weaverryan mentioned this pull request Jun 22, 2021

[TwigComponent] Improvements suggestion #108

Closed

dbrekelmans approved these changes Jun 23, 2021

View reviewed changes

weaverryan reviewed Jun 24, 2021

View reviewed changes

kbond force-pushed the attributes branch 3 times, most recently from 48f8f5e to 1eee8ae Compare June 25, 2021 23:37

kbond marked this pull request as ready for review June 27, 2021 13:58

kbond requested a review from weaverryan June 27, 2021 13:58

kbond force-pushed the attributes branch from 4b86fe5 to b397d83 Compare June 27, 2021 14:05

ker0x reviewed Jun 27, 2021

View reviewed changes

weaverryan reviewed Jun 28, 2021

View reviewed changes

dbrekelmans reviewed Jun 28, 2021

View reviewed changes

kbond force-pushed the attributes branch 2 times, most recently from d177254 to 4d3c8e8 Compare June 29, 2021 19:44

kbond requested a review from weaverryan June 29, 2021 19:57

weaverryan approved these changes Jun 29, 2021

View reviewed changes

kbond mentioned this pull request Jun 30, 2021

Update for twig/live component attribute refactor weaverryan/live-demo#4

Merged

kbond changed the title ~~[WIP] Twig/LiveComponent Attributes~~ Twig/LiveComponent Attributes Jun 30, 2021

Twig/LiveComponent Attributes

46777c3

weaverryan force-pushed the attributes branch from 4d3c8e8 to 46777c3 Compare July 1, 2021 12:50

weaverryan merged commit 4e7cd8c into symfony:main Jul 1, 2021

kbond deleted the attributes branch July 1, 2021 13:05

weaverryan mentioned this pull request Jul 1, 2021

Adding CHANGELOG for twig/live component #114

Merged

kbond mentioned this pull request Jul 1, 2021

[bug] remove duplicated doctrine/annotations entry #115

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Twig/LiveComponent Attributes #106

Twig/LiveComponent Attributes #106

kbond commented Jun 21, 2021 •

edited

Loading

weaverryan left a comment

wouterj commented Jun 21, 2021

dbrekelmans left a comment

kbond commented Jun 21, 2021 •

edited

Loading

weaverryan commented Jun 22, 2021

kbond commented Jun 22, 2021

kbond commented Jun 22, 2021

weaverryan commented Jun 23, 2021

weaverryan commented Jun 23, 2021

kbond commented Jun 23, 2021 •

edited

Loading

dbrekelmans commented Jun 23, 2021

kbond commented Jun 23, 2021

kbond commented Jun 23, 2021

weaverryan left a comment

kbond commented Jun 27, 2021 •

edited

Loading

ker0x Jun 27, 2021

kbond Jun 27, 2021

weaverryan left a comment

dbrekelmans left a comment

weaverryan left a comment

weaverryan commented Jul 1, 2021

Twig/LiveComponent Attributes #106

Twig/LiveComponent Attributes #106

Conversation

kbond commented Jun 21, 2021 • edited Loading

weaverryan left a comment

Choose a reason for hiding this comment

wouterj commented Jun 21, 2021

dbrekelmans left a comment

Choose a reason for hiding this comment

kbond commented Jun 21, 2021 • edited Loading

weaverryan commented Jun 22, 2021

kbond commented Jun 22, 2021

kbond commented Jun 22, 2021

weaverryan commented Jun 23, 2021

weaverryan commented Jun 23, 2021

kbond commented Jun 23, 2021 • edited Loading

dbrekelmans commented Jun 23, 2021

kbond commented Jun 23, 2021

kbond commented Jun 23, 2021

weaverryan left a comment

Choose a reason for hiding this comment

kbond commented Jun 27, 2021 • edited Loading

ker0x Jun 27, 2021

Choose a reason for hiding this comment

kbond Jun 27, 2021

Choose a reason for hiding this comment

weaverryan left a comment

Choose a reason for hiding this comment

dbrekelmans left a comment

Choose a reason for hiding this comment

weaverryan left a comment

Choose a reason for hiding this comment

weaverryan commented Jul 1, 2021

kbond commented Jun 21, 2021 •

edited

Loading

kbond commented Jun 21, 2021 •

edited

Loading

kbond commented Jun 23, 2021 •

edited

Loading

kbond commented Jun 27, 2021 •

edited

Loading