-
-
Notifications
You must be signed in to change notification settings - Fork 368
LiveProp in URL path #2673
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 2.x
Are you sure you want to change the base?
LiveProp in URL path #2673
Conversation
📊 Packages dist files size differenceThanks for the PR! Here is the difference in size of the packages dist files between the base branch and the PR.
|
(Had a quick chat with @mbuliard yesterday — he's making a few adjustments before opening the PR. Stay tuned! 😄 ) |
93c0a12
to
d848b54
Compare
Do you want/need any help here @mbuliard ? |
ceabe03
to
04dc91a
Compare
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 |
9393039
to
df97b2f
Compare
src/LiveComponent/src/EventListener/LiveComponentSubscriber.php
Outdated
Show resolved
Hide resolved
src/LiveComponent/tests/Unit/EventListener/LiveUrlSubscriberTest.php
Outdated
Show resolved
Hide resolved
Thanks for working on it! Just few nitpicking, the PR seems pretty mature right now |
… props also from path. RequestInitializeSubscriber also renamed
Thanks for your review ! I made the changes you suggested. |
src/LiveComponent/doc/index.rst
Outdated
// ... | ||
|
||
#[Route('/node/{id}', name: 'node')] | ||
public function node(NodeModule $nodeModule): Response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, you can (or must) inject the LiveComponent instance as a route parameter? 🤔
It seems weird to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to but indeed this looks weird (and I think led to my confusion on this feature initially).
What about /search/{mode}/{query}
? This way we can build upon the existing example in the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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=123
vshttp://example.com/content/123
To set the LiveProp in the path, a new option,
mapPath
has been added to the UrlMapping option of the LiveProp :WORKFLOW
X-Live-Url
containing the current path and query parameters.X-Live-Url
.history.replaceState
.BACKEND CHANGES
mapPath
, boolean, false by default.getAllUrlMappings
returning urlMappings of all LiveProps.X-Live-Url
of 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_data
and sorted between path-mapped and query-mapped.responseProps
data to the_live_request_data
attribute, containing the mounted component data when the action is not the default one. This change is made to take server-side changes into account.getAllUrlMappings
returning urlMappings of all LiveProps.FRONTEND CHANGES
X-Live-Url
header in the request.liveUrl
, populated from the HTTP responseX-Live-Url
header.performRequest
now check forX-Live-Url
header in response and, when found, dohistory.replace
with the new url and the current hash and origin.TODO
Review :-)