[LiveComponent] Allow binding LiveProp to URL query parameter #1230

squrious · 2023-10-25T07:50:57Z

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

Inspired from the Livewire Url attribute, let's bind our LiveProp props to the URL query string!

This is a first proposal, with the minimum feature behavior:

Add a new url constructor parameter to LiveProp
Bound props appear in the URL in a parameter with the same name
When the component is mounted for the first time (not in a live request), its props are initialized with provided query parameters
When the component is rendered on client side, the URL is updated according to the mapping (on first render and further live updates)

Currently supported data types:

Scalar props
Array props
Custom DTO objects with scalar and array properties

#[AsLiveComponent()]
final class MyComponent
{
    #[LiveProp(writable: true, url: true)]
    public ?string $search = null;

    #[LiveProp(writable: ['id', 'name'], url: true)]
    public ?SomeDto $dto = null;
    
    #[LiveProp(url: true)]
    public array $tags = [];
}

A bound LiveProp doesn't need to be writable, so it could be updated on server side based on a custom action.
BUT for DTOs, for the moment I bind the writable props only. Maybe another config option could be a better solution?

What could be next?

Improve data types support (nested DTOs, enums, specific properties from DTOs...)
Aliasing a prop name to a query parameter name (search => q, dto.id => some_id)
Add a keep option to force presence in the URL even if the prop is empty
Listen to external URL modification to synchronize props
Multiple history strategies (currently the URL is replaced, but we could push new entries instead)
Enable configuration from a getQueryString method in the component instead of attributes

You can also check my demo app to try the feature.

kbond · 2023-10-25T21:03:24Z

Nice work here! I'm curious how multiple components using this feature would behave. What about multiple of the same component?

weaverryan

SO excited for this! I'll do a more thorough review tomorrow

weaverryan · 2023-10-25T20:55:11Z

src/LiveComponent/src/EventListener/QueryStringInitializeSubscriber.php

+
+        $data = $event->getData();
+
+        $request = $this->requestStack->getCurrentRequest();


Should this be getMainRequest()? It would only make a difference if this component were being rendered inside of a sub-request. And in that case, we want to read the query param from the main, real request.

Yes it should, thanks! I change it

weaverryan · 2023-10-25T20:56:49Z

src/LiveComponent/src/EventListener/QueryStringInitializeSubscriber.php

+
+        $request = $this->requestStack->getCurrentRequest();
+
+        $queryStringData = $this->queryStringPropsExtractor->extract($request->getQueryString(), $metadata);


Does it make sense to pass $request->query->all() instead of the string (so we don't need to re-parse the string inside the extractor)?

Yes, and I'm about to pass the Request object directly, so the strategy to extract the data is fully managed in this class. I think it's better, wdyt?

That works for me 👍

squrious · 2023-10-26T08:57:35Z

Nice work here! I'm curious how multiple components using this feature would behave. What about multiple of the same component?
Thanks!

Hum that's actually a very good question 😄

I think this is related to the alias feature in some way; and we should be able to configure/override some part of the mapping in the component attributes. Like, maybe

{% component('MyComponent', { query-mapping: {"prop": "alias1}}) %}
{% component('MyComponent', { query-mapping: {"prop": "alias2}}) %}

squrious · 2023-10-26T10:08:43Z

Actually I'm wondering if the DTO support really worth it. For the moment it's quite limited:

As we should not use dots in query parameters, we use _. This is kind of naming strategy that should be abstracted.
Nested DTO properties are not allowed. This is mainly because I didn't yet implement any config to select some properties of a DTO prop, so it avoids problems such as self recursion etc.

The DTO support, if shipped with this feature, seems close to the Serializer concepts (max depth, projection, sub context, naming strategy, etc.). On the other hand, handling scalar values is really easy. So handling a custom DTO in a custom way, so the developer can eventually manage the scalar properties he/she wants, should be easy too.

That's all to say, I suggest to drop the incomplete support of DTOs, and instead provide an extension point (an interface or some tag injection) so :

The default extractor handles scalars and arrays of scalars
Additional strategies could be added in custom services by the developer

weaverryan · 2023-10-26T10:38:38Z

Actually I'm wondering if the DTO support really worth it. For the moment it's quite limited

My immediate reaction when I saw you had DTO support was "wow" but also "that sounds like it may get super complex" :).

That's all to say, I suggest to drop the incomplete support of DTOs, and instead provide an extension point (an interface or some tag injection) so :

Agreed! Though I am also not that concerned immediately about adding the extension point. If you have something obvious in mind, sure, let's add it. But it is not a need.

norkunas · 2023-10-26T10:57:33Z

Actually I'm wondering if the DTO support really worth it. For the moment it's quite limited:

As we should not use dots in query parameters, we use _. This is kind of naming strategy that should be abstracted.

Nested DTO properties are not allowed. This is mainly because I didn't yet implement any config to select some properties of a DTO prop, so it avoids problems such as self recursion etc.

The DTO support, if shipped with this feature, seems close to the Serializer concepts (max depth, projection, sub context, naming strategy, etc.). On the other hand, handling scalar values is really easy. So handling a custom DTO in a custom way, so the developer can eventually manage the scalar properties he/she wants, should be easy too.

That's all to say, I suggest to drop the incomplete support of DTOs, and instead provide an extension point (an interface or some tag injection) so :

The default extractor handles scalars and arrays of scalars

Additional strategies could be added in custom services by the developer

😞

Live component could set same urls as like submitting a form with GET method where query string is set to parent[field][..] etc. with brackets, I'd expect to work this same way, so I could refactor our live search to REAL live search :)

weaverryan · 2023-10-26T11:11:53Z

Live component could set same urls as like submitting a form with GET method where query string is set to parent[field][..] etc. with brackets, I'd expect to work this same way, so I could refactor our live search to REAL live search :)

As I said, I'm interested in getting a rich feature without going crazy on complexities. However, @norkunas may have a point here. We already have a dehydration system that is able to convert a DTO to an array of simple data - e.g. AddressDto dehydrates to ['street' => '404 Page Drive', 'city' => 'London', ...]. So, in theory, if AddressDto has url: true, could we set its "query string mapping" to be address[street]=404%20Page%20Drive&address[city]=London?

src/LiveComponent/src/Metadata/LivePropMetadata.php

weaverryan · 2023-10-26T10:42:39Z

src/LiveComponent/src/Util/QueryStringPropsExtractor.php

+
+        foreach ($metadata->getAllLivePropsMetadata() as $livePropMetadata) {
+            $queryStringBinding = $livePropMetadata->getQueryStringMapping();
+            foreach ($queryStringBinding['parameters'] ?? [] as $parameterName => $paramConfig) {


No need to code defensively here - if parameters is missing, it would be a bug in our code I think

I chose to always return an array from $livePropMetadata->getQueryStringMapping(), possibly populated with configuration values, such as parameters. So all unbound props have an empty array here. Maybe returning false instead of the empty array could be better?

I see - I'm ok with it.

weaverryan · 2023-10-26T10:44:14Z

src/LiveComponent/src/Util/QueryStringPropsExtractor.php

+            settype($value, $config['type']);
+        }
+
+        return $value;


Yup, I'd definitely like to see this simpler & supporting less complex cases :)

Using the LiveComponentHydrator simplified this part a lot!

weaverryan · 2023-10-26T10:52:50Z

src/LiveComponent/assets/src/Component/plugins/QueryStringPlugin.ts

+    }
+
+    private registerBindings(): void {
+        const rawQueryMapping = (this.element as HTMLElement).dataset.liveQueryMapping;


Hmm, probably better if we make this a real Stimulus value on the element. Then:

A) Read the value from the stimulus controller
B) Pass the mapping into the constructor of this class

I try to keep the Component class and plugins not ultimately dependent on Stimulus or the HTML structure as much as possible.

Done in 717f726!

Should we update the polling plugin too?

Hmm, I think so yes! But in a different PR :)

weaverryan · 2023-10-26T10:54:57Z

src/LiveComponent/assets/src/Component/plugins/QueryStringPlugin.ts

+        this.registerBindings();
+
+        component.on('connect', (component: Component) => {
+            this.updateUrl(component);


My impression from Livewire is that we would only update the URL on page load if they have a (future) keep: true option. But, by default, the URL only changes after the value has been changed on the page - https://livewire.laravel.com/docs/url#display-on-page-load

I wasn't sure about this one, Livewire illustrates the keep behavior with an initial empty value, and I also considered a non-empty initial value, that could be set on component mount for example, based on some context. But it may be too much for the moment, so let's wait for the keep implementation!

weaverryan · 2023-10-26T11:02:36Z

src/LiveComponent/assets/src/url_utils.ts

+            }
+        }
+    }
+}


My initial thought is that it looks like you've done a really nice job. If It helps as a reference, I know that Caleb from Livewire worked very hard on the more complex query param creation for Livewire 3 - https://github.com/livewire/livewire/blob/d4839e3b2c23fc71e615e68bc29ff4de95751810/js/plugins/history/index.js#L176

Depending on which more-complex values we handle (e.g. do we handle a LiveProp that is an array of strings? What about an array, or array of strings?), we'll need to make sure the query param format works.

Thanks for the reference! I'm about to setup more complex use cases to determine the limits of my current implementation. This will help a lot!

Can you add a little note, perhaps new the top of the file, with a link to this file in Livewire - something like:

Adapted from https://github.com/livewire/livewire/blob/d4839e3b2c23fc71e615e68bc29ff4de95751810/js/plugins/history/index.js#L176

It's nice to give Livewire the deserved nod and also will help us, in the future, answer "why did we do it this way?"

squrious · 2023-10-26T13:47:48Z

Live component could set same urls as like submitting a form with GET method where query string is set to parent[field][..] etc. with brackets, I'd expect to work this same way, so I could refactor our live search to REAL live search :)

As I said, I'm interested in getting a rich feature without going crazy on complexities. However, @norkunas may have a point here. We already have a dehydration system that is able to convert a DTO to an array of simple data - e.g. AddressDto dehydrates to ['street' => '404 Page Drive', 'city' => 'London', ...]. So, in theory, if AddressDto has url: true, could we set its "query string mapping" to be address[street]=404%20Page%20Drive&address[city]=London?

Seems legit, associative arrays should already be supported. That would have been a better solution that the underscores for the current DTO support. I'll take a look :)

WebMamba · 2023-10-27T10:00:50Z

We already have a dehydration system that is able to convert a DTO to an array of simple data - e.g. AddressDto dehydrates to ['street' => '404 Page Drive', 'city' => 'London', ...]. So, in theory, if AddressDto has url: true, could we set its "query string mapping" to be address[street]=404%20Page%20Drive&address[city]=London

I agree here, but I think we should do some refactoring on the Hydrator class instead of building new hydration logic. We don't want to maintain two hydrations logic since we want this feature to mirror the rest. So we should add some public function to hydrade or dehydrate function.

What do you think about making some refactoring? I have the following implementation in mind:

we keep the LiveComponentHydrator
this LiveComponentHydrator takes as argument a tag iterator of PropertyHydrator
a PropertyHydrator is an object that implemented the following interface:

  interface PropertyHydratorInterface
{
    public function hydrate(): mixed;

    public function dehydrate(): array;

    public function support(): bool;
}
```
- The `LiveComponentHydrator` foreach properties of the component loop over the tag iterator of `PropertyHydrator` find the one that support the property type.
This is a similar implementation we already did for the DoctrineHydrator. We can have the following `PropertyHydrator`:
DateTime, Enum, Scalar, Dto, Collection, Array.
What do you think?

squrious · 2023-10-30T16:20:29Z

Hi there!

I addressed most of your comments, in separated commits:

Pass query mapping with standard Stimulus value
Don't update the URL on first load even if some props are initialized on the PHP side
Use the component hydrator to parse query string data on first render

The latter now allows to initialize complex objects from the URL, with the prop[foo]=some+string&prop[bar]=42 syntax. It also simplifies a lot the piece of code for the mapping, as we don't need the type guessing logic anymore. I still need to set up more complex use cases to test edge cases 😉

weaverryan

Looking closer and closer

weaverryan · 2023-10-31T17:35:25Z

src/LiveComponent/assets/src/Component/plugins/QueryStringPlugin.ts

+import { PluginInterface } from './PluginInterface';
+import {
+    setQueryParam, removeQueryParam,
+} from '../../url_utils';


One line - sometimes I'll use multiple lines for a lot, but not this short

src/LiveComponent/assets/src/Component/plugins/QueryStringPlugin.ts

src/LiveComponent/src/EventListener/QueryStringInitializeSubscriber.php

weaverryan · 2023-10-31T18:00:27Z

src/LiveComponent/src/LiveComponentHydrator.php

@@ -410,7 +410,7 @@ private function dehydrateObjectValue(object $value, string $classType, ?string
        return $dehydratedObjectValues;
    }

-    private function hydrateValue(mixed $value, LivePropMetadata $propMetadata, object $parentObject): mixed
+    public function hydrateValue(mixed $value, LivePropMetadata $propMetadata, object $parentObject): mixed


We should add a phpdoc method description to this and it should be moved up above the private methods

weaverryan · 2023-10-31T18:02:31Z

src/LiveComponent/src/Util/QueryStringPropsExtractor.php

+
+        foreach ($metadata->getAllLivePropsMetadata() as $livePropMetadata) {
+            $queryStringBinding = $livePropMetadata->getQueryStringMapping();
+            foreach ($queryStringBinding['parameters'] ?? [] as $parameterName => $paramConfig) {


I see - I'm ok with it.

squrious · 2023-11-03T13:57:26Z

Hi! I made some updates, especially on the URL utils. I ported the code from Livewire as suggested, which brings some improvements:

No need to "expand" prop names anymore, we can set complex parameters directly in the URL. I guess it could also better handle dynamically added nested complex objects, as we don't need to know the data structure before updating query params.
Array values are explicitly indexed in the URL. That's fine but also required additional sort on server side, as InputBag::all() doesn't sort the values by itself.
Parameters order is kept between URL changes (former implementation always put objects and arrays at the end of the query string).

The only main change I made to the original code is to avoid null values in the URL. I found it annoying that setting a single property in a DTO brings all other uninitialized values in the query string. Furthermore, depending on the input type of the prop, nulls may be wrongly casted (got the case with a numeric input filled with 0 because of this).

Also, now the URL is updated once after render. But in Livewire, it seems it is updated on each prop change, because all props may have their own history strategy. Not sure about how we could handle this in our feature?

weaverryan · 2023-11-06T20:13:55Z

Also, now the URL is updated once after render. But in Livewire, it seems it is updated on each prop change, because all props may have their own history strategy. Not sure about how we could handle this in our feature?

Hmm, that's a good point. Though, if I have 2 props that both will use history: true, does Livewire actually add two entries into the history (even if both prop changes resulted from a single re-render)? You don't need to actually dig in and answer that, but that looks weird to me. If I change search and mode and then click a button to re-render, that should result in just one history entry.

But what if search is history: true and mode is history: false? Obviously, if I ONLY change mode, then no new history entry should be made. And if I ONLY change search, then a new history entry should be added. But what if I change both at the same time and then re-render? I think the correct answer is that one new history entry should be added. So, by this logic (and please tell me if you disagree), we should update the URL for all props at the same time. And if even one of the changed props has history: true, then we would use pushState() to add a new history entry.

squrious · 2023-11-07T13:20:21Z

I think the correct answer is that one new history entry should be added. So, by this logic (and please tell me if you disagree), we should update the URL for all props at the same time. And if even one of the changed props has history: true, then we would use pushState() to add a new history entry.

That seems to me the better way to go. Thanks for your advises! I keep this in mind for the future implementation of the history feature.

squrious · 2023-11-08T14:00:34Z

Nice work here! I'm curious how multiple components using this feature would behave. What about multiple of the same component?

Hello! I made some experimentation on my local project to handle the case of multiple components. I think we should handle an optional attribute to prefix all parameter names in the component (so it acts like a "scope"). Thought about something like:

<twig:MyComponent query-param-prefix='prefix_'>

So the URL looks like ?prefix_prop1=foo.

And maybe we could also leverage the special key attribute as a shortcut for a such attribute?

weaverryan · 2023-11-09T15:55:56Z

That seems reasonable. Though, if 2 components are on the same page and are using the url functionality, as long as their parameter names don't collide, won't it work already? If ComponentA is adding a search param and ComponentB is adding a page param, if the URL is currently ?search=hello&page=1 and ComponentA re-renders, will the current implementation "keep" the page param - e.g. update it to ?search=changed&page=1?

Also, I'm super busy this week - but this is an important PR. As far as you know, is this ready to go (other than needing a rebase - thanks!) and is it waiting for a detailed review?

squrious · 2023-11-09T17:02:41Z

That seems reasonable. Though, if 2 components are on the same page and are using the url functionality, as long as their parameter names don't collide, won't it work already? If ComponentA is adding a search param and ComponentB is adding a page param, if the URL is currently ?search=hello&page=1 and ComponentA re-renders, will the current implementation "keep" the page param - e.g. update it to ?search=changed&page=1?

Yes, I confirm that two different components can share the URL if their parameters don't collide. I added an implementation that uses the special query-param-prefix attribute to prefix parameters, and used key as a shortcut (but it is easily removable).

Also, I'm super busy this week - but this is an important PR. As far as you know, is this ready to go (other than needing a rebase - thanks!) and is it waiting for a detailed review?

Rebase done 😉

I think the current code runs well and is tested enough. We already discussed the expected behaviors a lot so it should be fine. But I added two more commits which may need a quick check, if you have the time of course!

aff83f2 : Simplify the mapping structure on PHP side, and fix some edge cases in URL data management on JS side
23d4b7c: The prefix attribute implementation mentioned above

Btw, I also worked on the other features (keep, alias, history) and they work pretty nicely. I can create a new PR when this one gets merged :)

weaverryan

Down to the small stuff. I played with this and it works FANTASTICALLY - super excited to have this!

weaverryan · 2023-11-13T17:57:42Z

src/LiveComponent/assets/src/Component/plugins/QueryStringPlugin.ts

+
+            Object.entries(this.mapping).forEach(([prop, mapping]) => {
+                const value = component.valueStore.get(prop);
+                if (this.isEmpty(value)) {


Can you explain the motivation for taking time to remove the "empty" query params? What if I have a LiveProp called search which defaults to foo in my PHP class. Then, the user changes an input bound to this to ''. If that removes ?search from the URL, then if I refresh, won't my search property once again be set to foo (since there was no query parameter to override it)?

I didn't consider this use case. You're right, providing an empty value in the URL should override values defined in PHP. The initial motivation for this check was to avoid empty values for objects and arrays. But I'll find a better way to manage these edge cases!

weaverryan · 2023-11-13T18:00:50Z

src/LiveComponent/assets/src/url_utils.ts

@@ -0,0 +1,138 @@
+function isObject(subject: any) {
+    return typeof subject === 'object' && subject !== null;
+}


function is only used once - let's inline the logic below where it's used

weaverryan · 2023-11-13T18:05:45Z

src/LiveComponent/assets/src/url_utils.ts

+            }
+        }
+    }
+}


Can you add a little note, perhaps new the top of the file, with a link to this file in Livewire - something like:

Adapted from https://github.com/livewire/livewire/blob/d4839e3b2c23fc71e615e68bc29ff4de95751810/js/plugins/history/index.js#L176

It's nice to give Livewire the deserved nod and also will help us, in the future, answer "why did we do it this way?"

weaverryan · 2023-11-13T18:18:34Z

src/LiveComponent/src/EventListener/QueryStringInitializeSubscriber.php

+    {
+        $metadata = new LiveComponentMetadata(
+            $event->getMetadata(),
+            $this->metadataFactory->createPropMetadatas(new \ReflectionClass($event->getComponent()::class))


Could it just be this?

$metadata = $this->metadataFactory->getMetadata($event->getMetadata()->getName());

I hoped that would have worked, but it breaks the hydrator tests because the tested component is mounted manually and not registered in the class map:

InvalidArgumentException: Unknown component "__testing". And no matching anonymous component template was found.

Is there something to fix here?

Is this only a tests problem ? If yes let's find a way to resolve it :)

What is blocking precisely ? Can i help you with something ?

Sorry I missed that one, I take a closer look

weaverryan · 2023-11-13T18:21:28Z

src/LiveComponent/src/EventListener/QueryStringInitializeSubscriber.php

+
+        $prefix = $data[LiveControllerAttributesCreator::URL_PREFIX_PROP_NAME]
+            ?? $data[LiveControllerAttributesCreator::KEY_PROP_NAME]
+            ?? '';


I'm not sold on this automatic collision avoidance. My gut is that I'd rather keep things simple. Then (later) give users the ability to control the name of the query parameter in the URL. Then, if they have a collision, they can decide which of the 2 colliding props to change the "parameter" for.

In think the prefix will still help in solving collisions if we have the same component rendered multiple times. Even if users can control the mapping from the template in the future, they will still need to override the full mapping, while a prefix easily solves the collision with a single attribute. And without the ability to override the mapping in attributes, it's a way to avoid collisions in other use cases.

However, I agree that adds more complexity to this first implementation, and maybe it's not worth it right now. If you prefer I can drop this attribute support until we have a better solution.

I'm worried about people getting confused when they're query params suddenly have a different name in the URL. Heck, it's also possible that people will purposely want to have a collision e.g. 2 separate search forms that both have query (though, they would need to do extra work to make it so that when the query changes in one component, it also updates the other - but this is totally possible)

Alright, I dropped the commit with the prefix management

weaverryan · 2023-11-13T18:24:58Z

src/LiveComponent/src/Util/QueryStringPropsExtractor.php

+    {
+    }
+
+    public function extract(Request $request, LiveComponentMetadata $metadata, object $component, string $prefix = ''): array


Add phpdoc:

Extracts relevant query parameters from the current URL & hydrates them.

weaverryan · 2023-11-13T18:26:30Z

src/TwigComponent/src/ComponentFactory.php

    {
-        $event = new PostMountEvent($component, $data);
+        $event = new PostMountEvent($component, $data, $componentMetadata);


Add a CHANGELOG entry for TwigComponents for this please!

weaverryan · 2023-11-13T18:30:42Z

src/TwigComponent/src/Event/PostMountEvent.php

@@ -21,6 +22,7 @@ final class PostMountEvent extends Event
    public function __construct(
        private object $component,
        private array $data,
+        private ComponentMetadata $metadata,
        private array $extraMetadata = [],
    ) {


This is a BC break, though almost definitely minor. But we should handle it. General steps:

A) Remove the types from the 3rd and 4th argument
B) If the 3rd argument is an array, trigger a deprecation - something like:

In TwigComponent 3.0, the 3rd argument of PostMountEvent will be a ComponentMetadata object.

And, in this case, set the 3rd arg $metadata will be set onto the $extraMetadata property (so, also, we will need to remove the private from args 3 and 4 and set them as normal properties).

Also in this situation, set the $metadata property to null. We'll need to make the return type on getMetadata() nullable for now.

If both $metadata and $extraMetadata are provided, should I enforce their types in the constructor body so the error comes from the constructor and not the assignment?

Yup. So, use an if statement and trigger a deprecation if they pass the "old" way of doing things. But then, if they just pass some bad value - e.g. they pass a string to the 3rd arg - which was NEVER legal - check that and throw an exception.

That also would require to check in LiveComponent if the installed TwigComponent has the good version (as they are two different packages)

weaverryan · 2023-11-13T18:31:09Z

src/TwigComponent/src/Event/PreMountEvent.php


 /**
 * @author Ryan Weaver <ryan@symfonycasts.com>
 */
 final class PreMountEvent extends Event
 {
-    public function __construct(private object $component, private array $data)
+    public function __construct(private object $component, private array $data, private ComponentMetadata $metadata)


Make this argument optional defaulting to null. If it's null, trigger a deprecation. Make the getter below have a nullable return type.

squrious · 2023-11-15T16:57:33Z

Down to the small stuff. I played with this and it works FANTASTICALLY - super excited to have this!

I'm glad to hear that!

Latest commit handles bc break for TwigComponent and fix empty params removal :)

weaverryan

Played with it again - I thing we're there, except for this last item I found :)

weaverryan · 2023-11-16T14:11:21Z

src/LiveComponent/src/EventListener/QueryStringInitializeSubscriber.php

+
+        $queryStringData = $this->queryStringPropsExtractor->extract($request, $metadata, $event->getComponent());
+
+        $event->setData(array_merge($event->getData(), $queryStringData));


We have a tricky problem here. Suppose I have:

#[LiveProp(writable: true, url: true)] public array $other = [];

Then I purposely go to /my-page?other=a-string. Obviously, this a-string is an invalid value. And so this shouldn't happen in practice. However, it currently triggers a 500 error:

As you can see the 500 comes from the mounting process. Normally, it is totally ok to fail with a 500 during the mounting process, as the developer is providing the mounting props directly (so if they mess something up, we want to tell them). But in this case, the mounted data is coming from the user. So if it's invalid, it needs to be ignored. To make things trickier, a could be mounted directly on a property, or via a setter or even by passing it as an argument to the mount() method.

Here's my idea, which I think will be solid enough. In QueryStringsPropsExtractor, after hydrating the data, we do a type-check on the property. If the type of the hydrated data is incompatible with the property type (if there is one), we do NOT add the hydrates value to $data. WDYT?

That makes sense for me! I updated the code with your suggestion. I also ignored issues from the hydrator itself to prevent errors when hydrating objects from invalid values. I ignored HydrationException errors, but I wonder if ignoring any error would be better.

smnandre · 2023-11-29T06:20:33Z

src/TwigComponent/src/Event/PostMountEvent.php

+                throw new \InvalidArgumentException(sprintf('Expecting "$extraMetadata" to be array, given: "%s".', get_debug_type($extraMetadata)));
+            }
+
+            $this->metadata = $metadata;


There is a regression here.

Before it was possible to do

new PostMountEvent($component, ['foo'])

I think we can do the following constructor to keep things working as before, and still a bit typed

public function __construct( private object $component, private array $data, array|ComponentMetadata $metadata = [], array $extraMetadata = [], ) {

smnandre · 2023-11-29T06:24:10Z

Sorry for the flood i did not start thinking i'd write many comments :)

smnandre · 2023-11-29T21:25:20Z

You can also check my demo app to try the feature.

Finally took the time to install it and play a bit and .... this demo is 🔥 !! (this PR too)

squrious · 2023-12-01T09:15:26Z

@smnandre Updates are coming, I compile all fixes in a single commit ;)

smnandre · 2023-12-01T17:56:43Z

src/LiveComponent/src/EventListener/QueryStringInitializeSubscriber.php

+    public static function getSubscribedEvents(): array
+    {
+        return [
+            PreMountEvent::class => 'onPreMount',


Can you add some tests to see how the different listener interfere (or not) with this listener ?

I think IntreceptChild / Defer / DataModel but maybe more, maybe less

And i think we should add priorities to ensure a determined order.

I don't think it would be useful to test with InterceptChild and Defer, as they don't subscribe to the PreMount event.

For DataModel, I'll have to setup some advanced use cases with parent and child components in order to test it efficiently.

My first thought is the query string initialization should come before the DataModel listener. The latter purpose is to set the child prop value from the parent bound prop, and I think replacing this value with the one extracted from the query string will lead the child component in an inconsistent state regarding its parent. Wdyt?

smnandre · 2023-12-01T17:58:13Z

src/LiveComponent/src/EventListener/QueryStringInitializeSubscriber.php

+            return;
+        }
+
+        $metadata = new LiveComponentMetadata(


I'm less and less convinced with this one... Could we use a single factory everywhere.. This new make me fear future problems if we add cache for example in a place and not the other one.

I just tried again with the factory and now it works, because we check if there is a request to process (wasn't checked last time I tried). Not sure if there is still something to fix now.

smnandre · 2023-12-01T18:26:43Z

Not sure yet about those things (is there something or ... not at all)

But could you explain me what happens if:

someone adds any query parameters to the _component endpoint ? should they be used ?
someone has a component with a non writable field BUT queryMapping = true ?
someone has a component with a writable field AND queryMapping = true ?

My intuition would be "it must be writable if a query can change that value" but i'm not sure if some use cases contredict that

On the opposite, i think the "standard" live hydration starts by setting values, before hydrating the "new ones"... Would that not interfere in a way with the query data ?

squrious · 2023-12-04T08:38:54Z

someone adds any query parameters to the _component endpoint ? should they be used ?

You mean the route used by the LiveComponent bundle to render components? I'd say it should not be processed at all.

My intuition would be "it must be writable if a query can change that value" but i'm not sure if some use cases contredict that

You can consider a prop that is not writable directly from the component but can be updated on server side. For example, an array whom items can be added through an input field and deleted with a button, but with a LiveAction. It is possible, even if I'd tend to avoid in favor of a stimulus controller.

On the opposite, i think the "standard" live hydration starts by setting values, before hydrating the "new ones"... Would that not interfere in a way with the query data ?

I don't think there could be interference here, because the LiveComponentHydrator is used only on live updates. The query data are used on the first load, and at this point the component is mounted with the regular TwigComponent's factory.

weaverryan

Thanks for keeping this up to date! For me, just one comment on naming. Then let's get this thing merged :)

weaverryan · 2023-12-12T15:18:57Z

src/LiveComponent/src/Attribute/LiveProp.php

+     *
+     * Tells if this property should be bound to the URL
+     */
+    private bool $queryMapping;


I know you and @smnandre discussed this change... but I did like url more. A url: true that, potentially later, could be changed to a bool|UrlMapping type with some new UrlMapping object. Then the user could have url: new UrlMapping(..).

Though, I am ok with keeping the "query mapping" or "url mapping" terminology deeper in the code.

weaverryan · 2023-12-18T19:22:58Z

Sorry - one last change :p. Can you rebase? We have a conflict, I believe, from #1341. Then we'll get this merged!

squrious · 2023-12-19T08:38:21Z

Sorry - one last change :p. Can you rebase? We have a conflict, I believe, from #1341. Then we'll get this merged!

Should be ok now ;)

weaverryan · 2023-12-19T18:53:26Z

Thank you 1 million for this HUGE effort and your constant responsiveness. I can't wait to use this! Cheers @squrious!

weaverryan · 2023-12-19T18:54:54Z

Btw, this does still needs some docs. @squrious do you want to make a PR for that? It doesn't need to be perfect - just getting something is 95% of the important part.

smnandre · 2023-12-19T20:43:22Z

I cannot say it better ! I have been really attentive to some details but this PR is really amazing and i’m so glad you worked as much to deliver. Congrats and thank you !

squrious · 2023-12-20T08:12:10Z

Thanks to you @weaverryan @smnandre for all the time you invested in the reviews! That was so cool to work on a such feature. I'll make the PR for the documentation ;)

lyrixx · 2024-01-04T11:58:43Z

src/LiveComponent/CHANGELOG.md

@@ -15,6 +15,7 @@
 -   Fix instantiating LiveComponentMetadata multiple times.
 -   Change JavaScript package to `type: module`.
 -   Throwing an error when setting an invalid model name.
+-   Add support for URL binding in `LiveProp`


I think this should be updated. It looks like it has not been released in v2.13.3: https://github.com/symfony/ux/blob/v2.13.3/src/LiveComponent/CHANGELOG.md

This PR was squashed before being merged into the 2.x branch. Discussion ---------- Add doc for URL binding feature | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | Issues | NA | License | MIT Add documentation for #1230 Commits ------- 4f0d313 Add doc for URL binding feature

This PR was squashed before being merged into the 2.x branch. Discussion ---------- Add doc for URL binding feature | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | Issues | NA | License | MIT Add documentation for symfony/ux#1230 Commits ------- 4f0d3132 Add doc for URL binding feature

This PR was squashed before being merged into the 2.x branch. Discussion ---------- [LiveComponent] Alias URL bound props | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | Issues | N/A | License | MIT Following #1230. Allow custom parameter names for URL bound props, and mapping specification from Twig templates. ## Usage From PHP definition: ```php #[AsLiveComponent()] final class MyComponent { // ... #[LiveProp(writable: true, url: new QueryMapping(alias: 'q')) public ?string $search = null; } ``` From templates: ```twig {{ component('MyComponent', { 'data-live-url-mapping-search': { 'alias': 'q' } }) }} {{ component('MyComponent', { 'data-live-url-mapping-search-alias': 'q' }) }} ``` HTML syntax also works: ```twig <twig:MyComponent :data-live-url-mapping-search="{ alias: 'q'}" /> <twig:MyComponent data-live-url-mapping-search-alias="q" /> ``` ## Result Changing the value of `search` will update the url to `https://my.domain?q=my+search+string`. Mappings provided in Twig templates are merged into those provided in PHP. Thus, query mappings in PHP act as defaults, and we can override them in templates (e.g. for specific page requirements). So a page with: ```twig <twig:MyComponent/> <twig:MyComponent data-live-url-mapping-search-alias="q" /> ``` will update its URL to `http://my.domain?search=foo&q=bar`. Commits ------- 828e34e [LiveComponent] Alias URL bound props

This PR was squashed before being merged into the 2.x branch. Discussion ---------- [LiveComponent] Alias URL bound props | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | Issues | N/A | License | MIT Following symfony/ux#1230. Allow custom parameter names for URL bound props, and mapping specification from Twig templates. ## Usage From PHP definition: ```php #[AsLiveComponent()] final class MyComponent { // ... #[LiveProp(writable: true, url: new QueryMapping(alias: 'q')) public ?string $search = null; } ``` From templates: ```twig {{ component('MyComponent', { 'data-live-url-mapping-search': { 'alias': 'q' } }) }} {{ component('MyComponent', { 'data-live-url-mapping-search-alias': 'q' }) }} ``` HTML syntax also works: ```twig <twig:MyComponent :data-live-url-mapping-search="{ alias: 'q'}" /> <twig:MyComponent data-live-url-mapping-search-alias="q" /> ``` ## Result Changing the value of `search` will update the url to `https://my.domain?q=my+search+string`. Mappings provided in Twig templates are merged into those provided in PHP. Thus, query mappings in PHP act as defaults, and we can override them in templates (e.g. for specific page requirements). So a page with: ```twig <twig:MyComponent/> <twig:MyComponent data-live-url-mapping-search-alias="q" /> ``` will update its URL to `http://my.domain?search=foo&q=bar`. Commits ------- 828e34e5 [LiveComponent] Alias URL bound props

squrious force-pushed the live-component/live-prop-url-binding branch 5 times, most recently from e25e877 to 42998d4 Compare October 25, 2023 14:08

weaverryan reviewed Oct 25, 2023

View reviewed changes

weaverryan reviewed Oct 26, 2023

View reviewed changes

weaverryan reviewed Oct 31, 2023

View reviewed changes

squrious force-pushed the live-component/live-prop-url-binding branch from db3eeb1 to 36fb1f0 Compare November 3, 2023 14:03

squrious force-pushed the live-component/live-prop-url-binding branch 2 times, most recently from 3517dcb to 23d4b7c Compare November 9, 2023 16:49

weaverryan reviewed Nov 13, 2023

View reviewed changes

squrious force-pushed the live-component/live-prop-url-binding branch from 23d4b7c to c73a24e Compare November 15, 2023 16:46

weaverryan reviewed Nov 16, 2023

View reviewed changes

smnandre reviewed Nov 29, 2023

View reviewed changes

smnandre reviewed Dec 1, 2023

View reviewed changes

squrious force-pushed the live-component/live-prop-url-binding branch from e700c92 to 9c1a62f Compare December 4, 2023 16:36

squrious force-pushed the live-component/live-prop-url-binding branch from 9c1a62f to 1301ae8 Compare December 12, 2023 09:31

weaverryan reviewed Dec 12, 2023

View reviewed changes

squrious force-pushed the live-component/live-prop-url-binding branch 3 times, most recently from 360282c to d894632 Compare December 19, 2023 08:32

[LiveComponent] Allow binding LiveProp to URL query parameter

68e4dd3

weaverryan force-pushed the live-component/live-prop-url-binding branch from d894632 to 68e4dd3 Compare December 19, 2023 18:52

weaverryan merged commit 524ac5a into symfony:2.x Dec 19, 2023
32 of 34 checks passed

squrious mentioned this pull request Dec 20, 2023

Add doc for URL binding feature #1346

Merged

lyrixx reviewed Jan 4, 2024

View reviewed changes

squrious mentioned this pull request Jan 16, 2024

[LiveComponent] Alias URL bound props #1396

Merged


		$data = $event->getData();

		$request = $this->requestStack->getCurrentRequest();


		$request = $this->requestStack->getCurrentRequest();

		$queryStringData = $this->queryStringPropsExtractor->extract($request->getQueryString(), $metadata);


		$queryStringData = $this->queryStringPropsExtractor->extract($request, $metadata, $event->getComponent());

		$event->setData(array_merge($event->getData(), $queryStringData));

[LiveComponent] Allow binding LiveProp to URL query parameter #1230

[LiveComponent] Allow binding LiveProp to URL query parameter #1230

Conversation

squrious commented Oct 25, 2023 • edited

kbond commented Oct 25, 2023

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

Choose a reason for hiding this comment

squrious commented Oct 26, 2023 • edited

squrious commented Oct 26, 2023

weaverryan commented Oct 26, 2023

norkunas commented Oct 26, 2023

weaverryan commented Oct 26, 2023

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squrious commented Oct 26, 2023

WebMamba commented Oct 27, 2023 • edited

squrious commented Oct 30, 2023

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

squrious commented Nov 3, 2023

weaverryan commented Nov 6, 2023

squrious commented Nov 7, 2023

squrious commented Nov 8, 2023

weaverryan commented Nov 9, 2023

squrious commented Nov 9, 2023

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

Choose a reason for hiding this comment

squrious Nov 14, 2023 • edited

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

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

Choose a reason for hiding this comment

squrious Nov 14, 2023 • edited

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 Nov 15, 2023

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

smnandre commented Nov 29, 2023

smnandre commented Nov 29, 2023

squrious commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squrious commented Oct 25, 2023 •

edited

squrious commented Oct 26, 2023 •

edited

WebMamba commented Oct 27, 2023 •

edited

squrious Nov 14, 2023 •

edited

squrious Nov 14, 2023 •

edited