[LiveComponent] Add modifier option to LiveProp #1507

squrious · 2024-02-16T10:01:57Z

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

Hi!

Following discussions in #1396, here is a proposal to allow a LiveProp to be modified at runtime.

The feature adds a modifier option to LiveProp, so users can use a custom method to modify the LiveProp's options
depending on the context.

This context could be a service, or even another prop within the component.

Usage

Here is the example given in the doc:

#[AsLiveComponent]
class ProductSearch
{
    #[LiveProp(writable: true, modifier: 'modifyAddedDate')]
    public ?\DateTimeImmutable $addedDate;

    #[LiveProp]
    public string $dateFormat = 'Y-m-d';

    // ...

    public function modifyAddedDate(LiveProp $prop): LiveProp
    {
        return $prop->withFormat($this->dateFormat);
    }
}

{{ component('ProductSearch', {
    dateFormat: 'd/m/Y'
}) }}

Changes overview

First, we have to make the LiveProp attribute editable in some way. I opted out for immutable methods, so caching is preserved when applicable.

Then add a LivePropMetadata::withModifier(object $component) method that returns a new instance modified by the user-defined method. This is called in a few places, similarly to LiveComponentMetadata::calculateFieldName().

The tricky part is component hydration. If we use props in modifiers to change other LiveProp options, they have to be hydrated before the modifier is applied. This is only useful for options related to hydration, but it concerns most of them... The trick is to sort metadata, so modified ones are hydrated at the end.

WebMamba

Ok, just let me summarize the approch here:
When a method have a modifier attribute, we execute the modifier function, get the modified value, we create a clone of the PropMetada with the new value. All of that happen during the hydratation.
What about the dehydratation?

src/LiveComponent/src/Attribute/LiveProp.php

WebMamba · 2024-02-18T15:19:23Z

src/LiveComponent/src/LiveComponentHydrator.php

+        uasort($allMetadata, static function (LivePropMetadata $a, LivePropMetadata $b) {
+            if ($a->hasModifier() && !$b->hasModifier()) {
+                return 1;
+            } elseif (!$a->hasModifier() && $b->hasModifier()) {
+                return -1;
+            } else {
+                return 0;
+            }
+        });


Why do we need to put the method with a modifier in priority of others?

Metadata with a modifier are considered "greater" than others, and the array is sorted in asc direction. I'd say this code prioritize metadata without modifier?

kbond · 2024-02-18T16:25:32Z

I actually had a need recently to only enable live props that update the URL under certain conditions. I believe this PR would allow that, correct?

squrious · 2024-02-19T08:22:32Z

I actually had a need recently to only enable live props that update the URL under certain conditions. I believe this PR would allow that, correct?

Yes totally!

When a method have a modifier attribute, we execute the modifier function, get the modified value, we create a clone of the PropMetada with the new value. All of that happen during the hydratation.
What about the dehydratation?

Modifiers are not only used on hydration. Actually this is used a bit like calculateFieldName: we need it applied on hydration and dehydration, but also in other places like when processing/rendering URL mappings.

weaverryan

Very tricky PR - I'm hoping we can make this work - excellent job 🤞

weaverryan · 2024-02-21T00:15:36Z

src/LiveComponent/doc/index.rst

+.. caution::
+
+    You must not rely on props that also use a modifier in other modifiers methods, as their modifiers will not
+    necessarily be applied in the correct order when hydrating components.


You must not rely on properties that also use a modifier in other modifiers methods. This is because those properties may not have been hydrated by this point.

weaverryan · 2024-02-21T00:31:40Z

src/LiveComponent/src/LiveComponentHydrator.php

@@ -66,6 +66,7 @@ public function dehydrate(object $component, ComponentAttributes $attributes, Li

        $dehydratedProps = new DehydratedProps();
        foreach ($componentMetadata->getAllLivePropsMetadata() as $propMetadata) {
+            $propMetadata = $propMetadata->withModifier($component);


This is the riskiest part of this PR. Currently, if you want the LivePropMetadata[] for a component, there is one source of truth: getAllLivePropsMetadata(). But now, someone needs to know to loop over these LivePropMetadata and call withModifier($component) to get the final version. That bothers me :/. I understand the "chicken and egg" problem, though: we need to hydrate the component (or as much of it as possible) before we can execute the modifier callbacks. I wish there was a way around that, but I can't think of anything more clever than your sorting algo.

However, to minimize the problem I mentioned above, would it be possible to add an object $component argument to getAllLivePropsMetadata() and make it return an iterable? Internally, it would do the sorting, then, one-by-one, call ->withModifier() and yield the final LiveProp. Would that work? Then everywhere in the code would call $componentMetadata->getAllLivePropsMetadata($component).

Just tested this, and it works perfectly!
But I suggest that we sort these LivePropMetadata in the LiveComponentMetadata constructor, so they are sorted once. As the modifier option is not editable, this should be fine, WDYT?

Yup, I think that's ok 👍

squrious · 2024-02-26T10:10:57Z

Modifiers are now applied each time getAllLivePropsMetadata is called. Maybe we could add an internal cache for this, based on spl_object_id?

I also had to move the listener for query string initialization to PostMount instead of PreMount, as it needs the component to be "pre-mounted" to know if some props are bound with modifiers. The test for this was not right in the first place... I set the priority to 256 to ensure it is executed before other custom listeners.

Some tests failed after rebasing, but I don't think this is related to this PR.

src/LiveComponent/doc/index.rst

src/LiveComponent/CHANGELOG.md

src/LiveComponent/src/EventListener/QueryStringInitializeSubscriber.php

weaverryan · 2024-02-29T15:03:31Z

src/LiveComponent/src/Util/LiveControllerAttributesCreator.php

                if ($livePropMetadata->queryStringMapping()) {
-                    $frontendName = $livePropMetadata->calculateFieldName($mounted, $livePropMetadata->getName());
+                    $frontendName = $livePropMetadata->calculateFieldName($mounted->getComponent(), $livePropMetadata->getName());


I'm confused by this change. Rather, it looks right now, but the old code looks totally wrong. Was this an unrelated bug that snuck in? And is it not tested?

I was confused by the (my) old code too. It's a bug that wasn't covered because I used a string field name in tests, and the component object is only used when the field name is computed by a method... Not directly related to this PR though, but I thought fixing the typo was worth it.

I added another prop in test component to test with the method for field name

weaverryan · 2024-02-29T15:08:30Z

Modifiers are now applied each time getAllLivePropsMetadata is called. Maybe we could add an internal cache for this, based on spl_object_id?

It sounds nice... but I don't know if it will make enough difference. Most components will not have a modifier, so the extra work will be a foreach over all the LiveProps & a call to the withModifier() method that exits immediately. That might (?) be something we should cache away... I just don't know.

squrious · 2024-02-29T16:39:27Z

It sounds nice... but I don't know if it will make enough difference. Most components will not have a modifier, so the extra work will be a foreach over all the LiveProps & a call to the withModifier() method that exits immediately. That might (?) be something we should cache away... I just don't know.

Yup, that's probably too much for the moment. I was wondering about modifiers that could use services, maybe heavy, to get configuration etc. But that could be cached in the component class directly. In the end, no need to generalize it 😄

weaverryan · 2024-03-05T16:56:06Z

Thanks Nicolas for your hard and careful work on this - well done!

carsonbot added the Status: Needs Review Needs to be reviewed label Feb 16, 2024

WebMamba reviewed Feb 18, 2024

View reviewed changes

weaverryan reviewed Feb 21, 2024

View reviewed changes

squrious force-pushed the live-component/live-prop-modifier branch 2 times, most recently from 20ead2c to 3ef4ace Compare February 26, 2024 10:09

weaverryan requested changes Feb 29, 2024

View reviewed changes

carsonbot added Status: Needs Work Additional work is needed and removed Status: Needs Review Needs to be reviewed labels Feb 29, 2024

carsonbot added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Feb 29, 2024

[LiveComponent] Add modifier option to LiveProp

3bcc5c2

weaverryan force-pushed the live-component/live-prop-modifier branch from 11b1ebc to 3bcc5c2 Compare March 5, 2024 16:55

weaverryan merged commit cae0f0e into symfony:2.x Mar 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LiveComponent] Add modifier option to LiveProp #1507

[LiveComponent] Add modifier option to LiveProp #1507

squrious commented Feb 16, 2024

WebMamba left a comment

WebMamba Feb 18, 2024

squrious Feb 19, 2024

kbond commented Feb 18, 2024

squrious commented Feb 19, 2024

weaverryan left a comment

weaverryan Feb 21, 2024

weaverryan Feb 21, 2024

squrious Feb 21, 2024

weaverryan Feb 29, 2024

squrious commented Feb 26, 2024

weaverryan Feb 29, 2024

squrious Feb 29, 2024

squrious Feb 29, 2024

weaverryan commented Feb 29, 2024

squrious commented Feb 29, 2024

weaverryan commented Mar 5, 2024

[LiveComponent] Add modifier option to LiveProp #1507

[LiveComponent] Add modifier option to LiveProp #1507

Conversation

squrious commented Feb 16, 2024

Usage

Changes overview

WebMamba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbond commented Feb 18, 2024

squrious commented Feb 19, 2024

weaverryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squrious commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Feb 29, 2024

squrious commented Feb 29, 2024

weaverryan commented Mar 5, 2024