Repository navigation
[LiveComponent] Add the possibility to map LiveProp as a path parameter - #2673
Conversation
📊 Packages dist files size differenceℹ️ No difference in dist packagesFiles. |
|
(Had a quick chat with @mbuliard yesterday — he's making a few adjustments before opening the PR. Stay tuned! 😄 ) |
|
Do you want/need any help here @mbuliard ? |
Thanks ! I needed free time, but the PR is almost done. Related tests are green, but I shall now add my own testing of the backend. What took me some time was to the handling of custom actions, who may (and often do) update component props. I choose to store the updated component props in |
|
Hey @squrious if you find a minute, i'd be happy to have your input (even a quick one) here :) |
carsonbot
added
Status: Needs Review
There was a problem hiding this comment.
Looks good! Couple questions in my review but also: Can/should you have multiple routes per component? Or should multiple LiveProp's with mapPath: true all be contained in a single route?
I think this should be clarified but also: it looks to me like this fully overrides the route for your component? If so, what about this concept being the feature?
|
@kbond
The |
|
@mbuliard, sorry, I misunderstood the feature. It took me a bit to realize the docs showed the route/controller that renders the component initially. |
My example was not clear enough, I added a line to clarify that it was a controller. |
|
Thanks for working on it! Just few nitpicking, the PR seems pretty mature right now |
Thanks for your review ! I made the changes you suggested. |
| $newUrl = null; | ||
| if ($previousLocation = $request->headers->get(self::URL_HEADER)) { | ||
| $liveProps = $this->getLivePropsToMap($request); | ||
| $newUrl = $this->urlFactory->createFromPreviousAndProps($previousLocation, $liveProps['path'], $liveProps['query']); | ||
| } | ||
|
|
||
| if ($newUrl) { | ||
| $event->getResponse()->headers->set(self::URL_HEADER, $newUrl); | ||
| } |
There was a problem hiding this comment.
| $newUrl = null; | |
| if ($previousLocation = $request->headers->get(self::URL_HEADER)) { | |
| $liveProps = $this->getLivePropsToMap($request); | |
| $newUrl = $this->urlFactory->createFromPreviousAndProps($previousLocation, $liveProps['path'], $liveProps['query']); | |
| } | |
| if ($newUrl) { | |
| $event->getResponse()->headers->set(self::URL_HEADER, $newUrl); | |
| } | |
| if ($previousLiveUrl = $request->headers->get(self::URL_HEADER)) { | |
| $liveProps = $this->getLivePropsToMap($request); | |
| $newLiveUrl = $this->urlFactory->createFromPreviousAndProps($previousLiveUrl, $liveProps['path'], $liveProps['query']); | |
| $event->getResponse()->headers->set(self::URL_HEADER, $newUrl); | |
| } |
There was a problem hiding this comment.
I kept two separated if to not set the header if the $newLiveUrl is null.
It can happen if :
- the previous url cannot be parsed
- the previous url does not match any known route
- some mandatory parameter is missing to generate the url of the route
But maybe it would be better to throw exceptions ?
I was worried to break anything doing so
Kocal
added
Status: Needs Work
carsonbot
added
Status: Reviewed
kbond
left a comment
kbond
left a comment
There was a problem hiding this comment.
Looks good! The docs are much clearer.
carsonbot
added
Status: Needs Work
carsonbot
added
Status: Reviewed
Kocal
changed the title
|
It took time, but here we go, this is in now. Thank you very much @mbuliard. |
…ng Serializer (and Serializer attributes), and when using custom modifier (Kocal) This PR was squashed before being merged into the 2.x branch. Discussion ---------- [LiveComponent] Fix new URL calculation with `LiveProp` using Serializer (and Serializer attributes), and when using custom modifier | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Docs? | no <!-- required for new features --> | Issues | Fix #2971, #2991 <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT <!-- Replace this notice by a description of your feature/bugfix. This will help reviewers and should be a good start for the documentation. Additionally (see https://symfony.com/releases): - Always add tests and ensure they pass. - For new features, provide some code snippets to help understand usage. - Features and deprecations must be submitted against branch main. - Update/add documentation as required (we can help!) - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry - Never break backward compatibility (see https://symfony.com/bc). --> This PR fixes two bugs introduced #2673: 1. See #2971 (comment) for explanations tl;dr: previously, query parameters were updated through `LiveComponentHydrator::dehydrate()` which use the Serializer (if `useSerializerForHydration: true`) and so also used Serializer attributes like `SerializedName`. 2. #2991, when having a `LiveProp(url: new UrlMapping(as: 'foo'))` with a custom modifier that returns a new `LiveProp` with a new `UrlMapping`, ex: ```php public function myModifier(LiveProp $liveProp): LiveProp { $urlMapping = $liveProp->url(); if (!$urlMapping instanceof UrlMapping) { return $liveProp; } return $liveProp->withUrl(new UrlMapping(as: 'alias_' . $urlMapping->as)); } ``` then the prop was persisted as `foo` and not `alias_foo` in the URL. Commits ------- 19fe704 [LiveComponent] Fix new URL calculation when having `#[LiveProp]` with custom `modifier` which returns a new `LiveProp` 07e73bf [LiveComponent] Fix new URL calculation when having `#[LiveProp]` with `useSerializerForHydration: true` and `#[SerializedName]`
…` with custom `fieldName` (Kocal) This PR was merged into the 2.x branch. Discussion ---------- [LiveComponent] Fix new URL generation when using `LiveProp` with custom `fieldName` | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Docs? | no <!-- required for new features --> | Issues | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT <!-- Replace this notice by a description of your feature/bugfix. This will help reviewers and should be a good start for the documentation. Additionally (see https://symfony.com/releases): - Always add tests and ensure they pass. - For new features, provide some code snippets to help understand usage. - Features and deprecations must be submitted against branch main. - Update/add documentation as required (we can help!) - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry - Never break backward compatibility (see https://symfony.com/bc). --> Follows #2673 Related to https://symfony-devs.slack.com/archives/C01FN4EQNLX/p1756395137633869?thread_ts=1756320013.552729&cid=C01FN4EQNLX, I believe this is the ultimate last fix for the `X-Live-Url` generation based on `LiveProp` 😅 I confirm that in <2.28, the URL query params were using the `fieldName` value from `LiveProp` if available, but we missed it (I didn't even know about that, the feature is not documented). Commits ------- 25c3e13 [LiveComponent] Fix new URL generation when using `LiveProp` with custom `fieldName`
5 participants
The purpose of this MR is to give the responsability of changing the URL to the backend, allowing to use Symfony Router and to have LiveProps in the path, not only in query parameters :
http://example.com/content?id=123vshttp://example.com/content/123To set the LiveProp in the path, a new option,
mapPathhas been added to the UrlMapping option of the LiveProp :WORKFLOW
X-Live-Urlcontaining the current path and query parameters.X-Live-Url.history.replaceState.BACKEND CHANGES
mapPath, boolean, false by default.getAllUrlMappingsreturning urlMappings of all LiveProps.X-Live-Urlof the response with the new URL, generated by theUrlFactory. To generate it, the previous location is extracted from the request and the props are extracted from metadata, hydrated with the values of_live_request_dataand sorted between path-mapped and query-mapped.responsePropsdata to the_live_request_dataattribute, containing the mounted component data when the action is not the default one. This change is made to take server-side changes into account.getAllUrlMappingsreturning urlMappings of all LiveProps.FRONTEND CHANGES
X-Live-Urlheader in the request.liveUrl, populated from the HTTP responseX-Live-Urlheader.performRequestnow check forX-Live-Urlheader in response and, when found, dohistory.replacewith the new url and the current hash and origin.TODO
Review :-)