Simplifies mapping and fix some edge cases

symfony · Nov 9, 2023 · aff83f2 · aff83f2
1 parent 1809b26
commit aff83f2
Show file tree

Hide file tree

Showing 10 changed files with 103 additions and 44 deletions.
diff --git a/src/LiveComponent/assets/dist/Component/plugins/QueryStringPlugin.d.ts b/src/LiveComponent/assets/dist/Component/plugins/QueryStringPlugin.d.ts
@@ -9,5 +9,6 @@ export default class implements PluginInterface {
         [p: string]: QueryMapping;
     });
     attachToComponent(component: Component): void;
+    private isEmpty;
 }
 export {};
diff --git a/src/LiveComponent/assets/dist/live_controller.js b/src/LiveComponent/assets/dist/live_controller.js
@@ -2707,11 +2707,14 @@ function toQueryString(data) {
         Object.entries(data).forEach(([iKey, iValue]) => {
             const key = baseKey === '' ? iKey : `${baseKey}[${iKey}]`;
             if (!isObject(iValue)) {
-                if (iValue !== null) {
+                if (null !== iValue) {
                     entries[key] = encodeURIComponent(iValue)
                         .replace(/%20/g, '+')
                         .replace(/%2C/g, ',');
                 }
+                else if ('' === baseKey) {
+                    entries[key] = '';
+                }
             }
             else {
                 entries = Object.assign(Object.assign({}, entries), buildQueryStringEntries(iValue, entries, key));
@@ -2731,22 +2734,22 @@ function fromQueryString(search) {
     const insertDotNotatedValueIntoData = (key, value, data) => {
         const [first, second, ...rest] = key.split('.');
         if (!second)
-            return (data[key] = value);
+            return data[key] = value;
         if (data[first] === undefined) {
-            data[first] = Number.isNaN(second) ? {} : [];
+            data[first] = Number.isNaN(Number.parseInt(second)) ? {} : [];
         }
         insertDotNotatedValueIntoData([second, ...rest].join('.'), value, data[first]);
     };
     const entries = search.split('&').map((i) => i.split('='));
     const data = {};
     entries.forEach(([key, value]) => {
-        if (!value)
-            return;
         value = decodeURIComponent(value.replace(/\+/g, '%20'));
         if (!key.includes('[')) {
             data[key] = value;
         }
         else {
+            if ('' === value)
+                return;
             const dotNotatedKey = key.replace(/\[/g, '.').replace(/]/g, '');
             insertDotNotatedValueIntoData(dotNotatedKey, value, data);
         }
@@ -2796,13 +2799,33 @@ class QueryStringPlugin {
             const urlUtils = new UrlUtils(window.location.href);
             const currentUrl = urlUtils.toString();
             Object.entries(this.mapping).forEach(([prop, mapping]) => {
-                urlUtils.set(mapping.name, component.valueStore.get(prop));
+                const value = component.valueStore.get(prop);
+                if (this.isEmpty(value)) {
+                    urlUtils.remove(mapping.name);
+                }
+                else {
+                    urlUtils.set(mapping.name, value);
+                }
             });
             if (currentUrl !== urlUtils.toString()) {
                 HistoryStrategy.replace(urlUtils);
             }
         });
     }
+    isEmpty(value) {
+        if (null === value || value === '' || value === undefined || Array.isArray(value) && value.length === 0) {
+            return true;
+        }
+        if (typeof value !== 'object') {
+            return false;
+        }
+        for (let key of Object.keys(value)) {
+            if (!this.isEmpty(value[key])) {
+                return false;
+            }
+        }
+        return true;
+    }
 }
 
 const getComponent = (element) => LiveControllerDefault.componentRegistry.getComponent(element);

diff --git a/src/LiveComponent/assets/src/Component/plugins/QueryStringPlugin.ts b/src/LiveComponent/assets/src/Component/plugins/QueryStringPlugin.ts
@@ -18,7 +18,12 @@ export default class implements PluginInterface {
             const currentUrl = urlUtils.toString();
 
             Object.entries(this.mapping).forEach(([prop, mapping]) => {
-                urlUtils.set(mapping.name, component.valueStore.get(prop));
+                const value = component.valueStore.get(prop);
+                if (this.isEmpty(value)) {
+                    urlUtils.remove(mapping.name);
+                } else {
+                    urlUtils.set(mapping.name, value);
+                }
             });
 
             // Only update URL if it has changed
@@ -27,4 +32,22 @@ export default class implements PluginInterface {
             }
         });
     }
+
+    private isEmpty(value: any): boolean
+    {
+        if (null === value || value === '' || value === undefined || Array.isArray(value) && value.length === 0) {
+            return true;
+        }
+
+        if (typeof value !== 'object') {
+            return false;
+        }
+
+        for (let key of Object.keys(value)) {
+            if (!this.isEmpty(value[key])) {
+                return false;
+            }
+        }
+        return true;
+    }
 }
diff --git a/src/LiveComponent/assets/src/url_utils.ts b/src/LiveComponent/assets/src/url_utils.ts
@@ -15,10 +15,13 @@ function toQueryString(data: any) {
             const key = baseKey === '' ? iKey : `${baseKey}[${iKey}]`;
 
             if (!isObject(iValue)) {
-                if (iValue !== null) {
+                if (null !== iValue) {
                     entries[key] = encodeURIComponent(iValue)
                         .replace(/%20/g, '+') // Conform to RFC1738
                         .replace(/%2C/g, ',');
+                } else if ('' === baseKey) {
+                    // Keep empty values for top level data
+                    entries[key] = '';
                 }
             } else {
                 entries = { ...entries, ...buildQueryStringEntries(iValue, entries, key) };
@@ -51,11 +54,11 @@ function fromQueryString(search: string) {
         const [first, second, ...rest] = key.split('.');
 
         // We're at a leaf node, let's make the assigment...
-        if (!second) return (data[key] = value);
+        if (!second) return data[key] = value;
 
         // This is where we fill in empty arrays/objects along the way to the assigment...
         if (data[first] === undefined) {
-            data[first] = Number.isNaN(second) ? {} : [];
+            data[first] = Number.isNaN(Number.parseInt(second)) ? {} : [];
         }
 
         // Keep deferring assignment until the full key is built up...
@@ -67,14 +70,14 @@ function fromQueryString(search: string) {
     const data: any = {};
 
     entries.forEach(([key, value]) => {
-        // Query string params don't always have values... (`?foo=`)
-        if (!value) return;
-
         value = decodeURIComponent(value.replace(/\+/g, '%20'));
 
         if (!key.includes('[')) {
             data[key] = value;
         } else {
+            // Skip empty nested data
+            if ('' === value) return;
+
             // Convert to dot notation because it's easier...
             const dotNotatedKey = key.replace(/\[/g, '.').replace(/]/g, '');
 

diff --git a/src/LiveComponent/assets/test/url_utils.test.ts b/src/LiveComponent/assets/test/url_utils.test.ts
@@ -35,12 +35,24 @@ describe('url_utils', () => {
                 expect(urlUtils.search).toEqual('?param=bar');
             });
 
+            it('preserve empty values if the param is scalar', () => {
+                urlUtils.set('param', '');
+
+                expect(urlUtils.search).toEqual('?param=');
+            });
+
             it('expand arrays in the URL', () => {
                 urlUtils.set('param', ['foo', 'bar']);
 
                 expect(urlUtils.search).toEqual('?param[0]=foo&param[1]=bar');
             });
 
+            it('prevent empty values if the param is array', () => {
+                urlUtils.set('param', []);
+
+                expect(urlUtils.search).toEqual('');
+            });
+
             it('expand objects in the URL', () => {
                 urlUtils.set('param', {
                     foo: 1,
@@ -49,6 +61,21 @@ describe('url_utils', () => {
 
                 expect(urlUtils.search).toEqual('?param[foo]=1&param[bar]=baz');
             });
+
+            it('remove empty values in nested object properties', () => {
+                urlUtils.set('param', {
+                    foo: null,
+                    bar: 'baz',
+                });
+
+                expect(urlUtils.search).toEqual('?param[bar]=baz');
+            });
+
+            it('prevent empty values if the param is an empty object', () => {
+                urlUtils.set('param', {});
+
+                expect(urlUtils.search).toEqual('');
+            });
         });
 
         describe('remove', () => {

diff --git a/src/LiveComponent/src/EventListener/QueryStringInitializeSubscriber.php b/src/LiveComponent/src/EventListener/QueryStringInitializeSubscriber.php
@@ -27,11 +27,6 @@
  */
 class QueryStringInitializeSubscriber implements EventSubscriberInterface
 {
-    /**
-     * @var array<class-string,LiveComponentMetadata>
-     */
-    private array $registered = [];
-
     public function __construct(
         private readonly RequestStack $requestStack,
         private readonly LiveComponentMetadataFactory $metadataFactory,

diff --git a/src/LiveComponent/src/Metadata/LiveComponentMetadataFactory.php b/src/LiveComponent/src/Metadata/LiveComponentMetadataFactory.php
@@ -136,15 +136,9 @@ private function createQueryStringMapping(string $propertyName, LiveProp $livePr
             return [];
         }
 
-        $queryStringMapping = [];
-
-        $queryStringMapping['parameters'] = [
-            $propertyName => [
-                'property' => $propertyName,
-            ],
+        return [
+            'name' => $propertyName,
         ];
-
-        return $queryStringMapping;
     }
 
     public function reset(): void

diff --git a/src/LiveComponent/src/Util/LiveControllerAttributesCreator.php b/src/LiveComponent/src/Util/LiveControllerAttributesCreator.php
@@ -126,11 +126,7 @@ public function attributesForRendering(MountedComponent $mounted, ComponentMetad
             $queryMapping = [];
             foreach ($liveMetadata->getAllLivePropsMetadata() as $livePropMetadata) {
                 if ($mapping = $livePropMetadata->getQueryStringMapping()) {
-                    foreach ($mapping['parameters'] as $parameter => $config) {
-                        $queryMapping[$config['property']] = [
-                            'name' => $parameter,
-                        ];
-                    }
+                    $queryMapping[$livePropMetadata->getName()] = $mapping;
                 }
             }
             $attributesCollection->setQueryUrlMapping($queryMapping);

diff --git a/src/LiveComponent/src/Util/QueryStringPropsExtractor.php b/src/LiveComponent/src/Util/QueryStringPropsExtractor.php
@@ -38,13 +38,12 @@ public function extract(Request $request, LiveComponentMetadata $metadata, objec
         $data = [];
 
         foreach ($metadata->getAllLivePropsMetadata() as $livePropMetadata) {
-            $queryStringBinding = $livePropMetadata->getQueryStringMapping();
-            foreach ($queryStringBinding['parameters'] ?? [] as $parameterName => $paramConfig) {
-                if (null !== ($value = $query[$parameterName] ?? null)) {
+            if ($queryStringMapping = $livePropMetadata->getQueryStringMapping()) {
+                if (null !== ($value = $query[$queryStringMapping['name']] ?? null)) {
                     if (\is_array($value) && $this->isNumericIndexedArray($value)) {
                         ksort($value);
                     }
-                    $data[$paramConfig['property']] = $this->hydrator->hydrateValue($value, $livePropMetadata, $component);
+                    $data[$livePropMetadata->getName()] = $this->hydrator->hydrateValue($value, $livePropMetadata, $component);
                 }
             }
         }

diff --git a/src/LiveComponent/tests/Functional/Metadata/LiveComponentMetadataFactoryTest.php b/src/LiveComponent/tests/Functional/Metadata/LiveComponentMetadataFactoryTest.php
@@ -31,23 +31,21 @@ public function testQueryStringMapping()
         }
 
         $this->assertEquals([
-            'parameters' => [
-                'prop1' => ['property' => 'prop1'],
-            ],
+            'name' => 'prop1',
         ], $propsMetadataByName['prop1']->getQueryStringMapping());
 
         $this->assertEquals([
-            'parameters' => [
-                'prop2' => ['property' => 'prop2'],
-            ],
+            'name' => 'prop2',
         ], $propsMetadataByName['prop2']->getQueryStringMapping());
 
         $this->assertEquals([
-            'parameters' => [
-                'prop3' => ['property' => 'prop3'],
-            ],
+            'name' => 'prop3',
         ], $propsMetadataByName['prop3']->getQueryStringMapping());
 
         $this->assertEquals([], $propsMetadataByName['prop4']->getQueryStringMapping());
+
+        $this->assertEquals([
+            'name' => 'prop5',
+        ], $propsMetadataByName['prop5']->getQueryStringMapping());
     }
 }