From aff83f258ce1e0b98a3122e7aceae22900088214 Mon Sep 17 00:00:00 2001 From: Nicolas Rigaud Date: Tue, 7 Nov 2023 17:09:09 +0100 Subject: [PATCH] Simplifies mapping and fix some edge cases --- .../Component/plugins/QueryStringPlugin.d.ts | 1 + .../assets/dist/live_controller.js | 35 +++++++++++++++---- .../Component/plugins/QueryStringPlugin.ts | 25 ++++++++++++- src/LiveComponent/assets/src/url_utils.ts | 15 ++++---- .../assets/test/url_utils.test.ts | 27 ++++++++++++++ .../QueryStringInitializeSubscriber.php | 5 --- .../Metadata/LiveComponentMetadataFactory.php | 10 ++---- .../Util/LiveControllerAttributesCreator.php | 6 +--- .../src/Util/QueryStringPropsExtractor.php | 7 ++-- .../LiveComponentMetadataFactoryTest.php | 16 ++++----- 10 files changed, 103 insertions(+), 44 deletions(-) diff --git a/src/LiveComponent/assets/dist/Component/plugins/QueryStringPlugin.d.ts b/src/LiveComponent/assets/dist/Component/plugins/QueryStringPlugin.d.ts index 3a6ff1319ec..ae0e3da511b 100644 --- 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 index e8b55134156..8986e513a23 100644 --- 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 index 13be77c0668..65f2cc9edcb 100644 --- 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 index 21b08a8c68a..a233b407142 100644 --- 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 index 4b098b8b46c..e15ce0708ae 100644 --- 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¶m[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¶m[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 index 997c8fcbf71..bc957ccc909 100644 --- a/src/LiveComponent/src/EventListener/QueryStringInitializeSubscriber.php +++ b/src/LiveComponent/src/EventListener/QueryStringInitializeSubscriber.php @@ -27,11 +27,6 @@ */ class QueryStringInitializeSubscriber implements EventSubscriberInterface { - /** - * @var array - */ - 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 index 51423ca6657..356fa80ad2d 100644 --- 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 index 14123745df6..0a13349de3c 100644 --- 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 index 71cfb094d82..74fe010aa82 100644 --- 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 index 198368cfb2d..996194b2295 100644 --- 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()); } }