-
-
Notifications
You must be signed in to change notification settings - Fork 315
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
Add deferred live components #1143
Add deferred live components #1143
Conversation
eaf7301
to
af5f0cf
Compare
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've left some notes - but this is wonderful! The name "lazy" is trendier... but defer might be slightly more accurate. Thinking ahead: if we, in the future, extended this to "don't load until the element is in the viewport", how would we name that?
src/LiveComponent/src/EventListener/AddLiveAttributesSubscriber.php
Outdated
Show resolved
Hide resolved
src/LiveComponent/src/EventListener/AddLiveAttributesSubscriber.php
Outdated
Show resolved
Hide resolved
// if the original attributes has a data-live-id, it means the component | ||
// was already rendered | ||
if (!$hasLiveId && $isDeferred) { | ||
$attributes = $attributes->without('defer', 'defer-loading-template'); |
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'd like the shorter loading-template
name... or maybe loadingTemplate
... we don't do any transformations from foo-bar
in HTML -> to fooBar
as a property, so we're typically using camel case attribute names... since they don't actually render in HTML.
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 decided to use loading-template
as an attribute name, but in the template it's loadingTemplate
. kebab-case
is a common pick for HTML, so I guess it fits perfectly here.
// was already rendered | ||
if (!$hasLiveId && $isDeferred) { | ||
$attributes = $attributes->without('defer', 'defer-loading-template'); | ||
$event->setTemplate('@LiveComponent/deferred.html.twig'); |
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.
It could be outside of the scope of this PR, but it'd be great to have some semantic configuration so you could control the default loading template globally. I have some semantic config introduced in #1140, so it might make sense to wait for that before adding this new option (e.g. loading_template
)
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 like it! Though, I do think we're going to need to remove the loading-template
from the data-live-props-value
. Both for being responsible/clean... but also not exposing template paths to the frontend.
src/LiveComponent/src/EventListener/AddLiveAttributesSubscriber.php
Outdated
Show resolved
Hide resolved
I guess we can call it |
7f62a42
to
fea1c7c
Compare
Indeed, I think |
@@ -0,0 +1,7 @@ | |||
<{{ loadingTag }} {{ attributes }} data-action="live:connect->live#$render"> | |||
{% block loadingContent %} |
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.
Would this working ?
<twig:Notifications defer>
<p>Loading...</p>
</twig:Notifications>
If not, i thing this could be a great DX addition... wdyt ?
If you'd want to use the "default" content block you'd just have to name it
<twig:Notifications defer>
{# Displayed on page load #}
<p>Loading...</p>
{# Displayed on demand #}
<twig:block name="content">
No notification
</twig:block>
</twig:Notifications>
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've checked this trying to do
<twig:SomeLiveComponent lazy>
{% block loadingContent %}
Loading...
{% endblock %}
</twig:SomeLiveComponent>
But it renders SomeLiveComponent
instantly 🥲. I'll try to debug how it works, maybe some adjustments and it'll work.
So, in the future, to be deferred and lazy, you'd need the following? <twig:MyComponent defer lazy /> I think that would work best as we could utilize lazy with polling: <twig:MyComponent defer lazy data-poll /> The above would only load when in the viewport and then only poll when still in the viewport. |
I guess |
I think we'd need both to allow the following to be possibly: <twig:MyComponent lazy data-poll /> This would load the component initially, then only poll when in the viewport. |
Question: should this be |
My (very personal) interpretation:
So "defer" is more to me like the kernel.terminate.. and lazy quite working there.. Two other ideas:
|
The name
The component is "executed" after the page loads (and it's also "downloaded" then in our case 😅). In terms of |
I was imagining these as 2 different mechanism. The ability to "only poll when in viewport" would be a property of the <twig:MyComponent lazy data-poll="lazy|$render" /> Yes, that |
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.
Can you take a shot at some docs also?
Thanks!
src/LiveComponent/src/EventListener/DeferLiveComponentSubscriber.php
Outdated
Show resolved
Hide resolved
|
||
$mountedComponent->setAttributes( | ||
$mountedComponent->getAttributes()->without('defer', 'loading-template', 'loading-tag'), | ||
); |
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.
@kbond does this look clunky to you? The fact that we need to... kind of modify the attributes in $variables[$attributesKey]
AND also on MountedComponent
?
Should we be doing this in an earlier hook? Something that can intercept the defer
input prop earlier and remove it? Maybe set that data onto MountedComponent::extraMetadata()
so it can be read here to set the variables?
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.
Hmm, perhaps, and I agree it is clunky but it does work within the current feature set. Opportunity for a future refactor.. I know I've said this for a few things recently. Maybe ugly === ok until we need to duplicate it elsewhere?
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.
Yea, I definitely have that on my mind too - finding the balance. What about:
A) In PostMountEvent
https://github.com/symfony/ux/blob/2.x/src/TwigComponent/src/ComponentFactory.php#L195 - we add a new addExtraMetadata()
method to the event.
B) Then, we keep track of that and put it into the MountedComponent
- https://github.com/symfony/ux/blob/2.x/src/TwigComponent/src/ComponentFactory.php#L91-L113 - a little odd, as we need to carry return that "extra metadata" from the private postMount()
, but I can't think of a better way. The user can intercept and remove "extra keys" from PostMount
... but they don't have access to "attach" that to the MountedComponent
.
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.
Yeah, sounds like that would work. I guess I'd be a little concerned about splitting this logic into two places (assuming I am understanding correctly). It's clunky here but at least it's in one spot (which makes it easier to reason about imo).
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.
Gotcha. I'm leaning towards my solution. It just doesn't feel right to allow defer
to get ALL the way into attributes... just for us to remove it later, because it is never meant to be an attribute. It is really a special "prop", which should be processed and removed before it ever gets into attributes.
Sorry for the back and forth @jakubtobiasz but does this make sense? It would allow us to remove the new method in MountedComponent
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'm leaning towards my solution
I'm ok with this - no strong objection.
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'll try to take a look this week 🫡. I'll reach you once I find some difficulties 😂.
6ce07d0
to
213832a
Compare
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 like this a lot more - THANK you :). Can you add a small section in the docs for this too? I'm thinking a new -----
section (like Polling) right above Polling https://symfony.com/bundles/ux-live-component/current/index.html#polling called Lazy-Loaded / Deferred Components
} | ||
|
||
$event->setVariables($variables); | ||
} |
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.
😻 This feels MUCH better to me.
src/LiveComponent/src/EventListener/DeferLiveComponentSubscriber.php
Outdated
Show resolved
Hide resolved
src/LiveComponent/src/EventListener/DeferLiveComponentSubscriber.php
Outdated
Show resolved
Hide resolved
213832a
to
2280d97
Compare
2280d97
to
d93cc29
Compare
Woohoo! THANK you for this wonderful work and feature Jacob! |
This PR was squashed before being merged into the 2.x branch. Discussion ---------- [LiveComponent] Tweaking the defer docs | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | Tickets | None | License | MIT Just some follow-up from #1143 Commits ------- 2f9ef84 [LiveComponent] Tweaking the defer docs
Initially this feature was called
lazy loading
, but this name could be misleading, as my solution doesn't provide real lazy loading. So, I named itdeferred live components
, I guess this name fits better.How to use it?
In such case we'll get an empty div. Once
live:connect
event called, it'll load the component in the background. We can also define a template to be rendered while loading (e.g. a cool spinner or some text).I'm open for any suggestions 🙌🏼!