Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add deferred live components #1143

Merged
merged 1 commit into from Oct 12, 2023

Conversation

jakubtobiasz
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
Tickets Fix #994
License MIT

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 it deferred live components, I guess this name fits better.

How to use it?

<twig:MyComponent defer />

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).

<twig:MyComponent defer defer-loading-template="my_cool_spinner.html.twig" />

I'm open for any suggestions 馃檶馃徏!

@jakubtobiasz jakubtobiasz changed the title Add a test case covering deferred components Add deferred live components Sep 25, 2023
Copy link
Member

@weaverryan weaverryan left a 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?

// 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');
Copy link
Member

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.

Copy link
Contributor Author

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.

src/LiveComponent/templates/deferred.html.twig Outdated Show resolved Hide resolved
// was already rendered
if (!$hasLiveId && $isDeferred) {
$attributes = $attributes->without('defer', 'defer-loading-template');
$event->setTemplate('@LiveComponent/deferred.html.twig');
Copy link
Member

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)

src/LiveComponent/templates/deferred.html.twig Outdated Show resolved Hide resolved
Copy link
Member

@weaverryan weaverryan left a 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.

@jakubtobiasz
Copy link
Contributor Author

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?

I guess we can call it lazy then. Or maybe there's a case to keep both, and have the current way (defer) and the new one (lazy).

@weaverryan
Copy link
Member

Indeed, I think defer better communicates what this does and lazy would better describe the viewport idea.

@@ -0,0 +1,7 @@
<{{ loadingTag }} {{ attributes }} data-action="live:connect->live#$render">
{% block loadingContent %}
Copy link
Collaborator

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>

Copy link
Contributor Author

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.

@kbond
Copy link
Member

kbond commented Sep 26, 2023

Indeed, I think defer better communicates what this does and lazy would better describe the viewport idea.

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.

@jakubtobiasz
Copy link
Contributor Author

So, in the future, to be deferred and lazy, you'd need the following?

I guess deferred or lazy should be enough 馃.

@kbond
Copy link
Member

kbond commented Sep 26, 2023

I guess deferred or lazy should be enough 馃.

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.

@kbond
Copy link
Member

kbond commented Sep 30, 2023

Question: should this be data-defer? If I understand correctly, since the defer attribute isn't rendered in the actual browser dom, it isn't an issue but... consistency? I don't have a strong opinion myself, just wanted to bring this up for discussion. Assuming we add a lazy attribute later for viewport/current tab detection, this will need to be data-lazy (and we already have data-poll).

@smnandre
Copy link
Collaborator

smnandre commented Oct 1, 2023

Question: should this be data-defer? If I understand correctly, since the defer attribute isn't rendered in the actual browser dom, it isn't an issue but... consistency? I don't have a strong opinion myself, just wanted to bring this up for discussion. Assuming we add a lazy attribute later for viewport/current tab detection, this will need to be data-lazy (and we already have data-poll).

My (very personal) interpretation:

  • eager = "now"
  • defer = "bottom of the todo-list"
  • lazy = "much later / when it'll be needed"

So "defer" is more to me like the kernel.terminate.. and lazy quite working there..

Two other ideas:

  • "async" simple and not related to "when" it will be renderer..
  • data-render=live

@jakubtobiasz
Copy link
Contributor Author

The name defer is inspired by attributes available for script tag.

If async is present: The script is downloaded in parallel to parsing the page, and executed as soon as it is available (before parsing completes)
If defer is present (and not async): The script is downloaded in parallel to parsing the page, and executed after the page has finished parsing

The component is "executed" after the page loads (and it's also "downloaded" then in our case 馃槄).

In terms of data- prefix, I guess we get get rid of it in the Twig files, and add this prefix only in the rendered template in all cases 馃.

@weaverryan
Copy link
Member

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.

I was imagining these as 2 different mechanism. The ability to "only poll when in viewport" would be a property of the data-poll specifically - so something like;

<twig:MyComponent lazy data-poll="lazy|$render" />

Yes, that |$render at the end is a little bulky. But the point is, lazy is a modifier of the polling.

Copy link
Member

@weaverryan weaverryan left a 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!


$mountedComponent->setAttributes(
$mountedComponent->getAttributes()->without('defer', 'loading-template', 'loading-tag'),
);
Copy link
Member

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?

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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).

Copy link
Member

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

Copy link
Member

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.

Copy link
Contributor Author

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 馃槀.

Copy link
Member

@weaverryan weaverryan left a 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);
}
Copy link
Member

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/TwigComponent/src/MountedComponent.php Outdated Show resolved Hide resolved
@weaverryan
Copy link
Member

Woohoo! THANK you for this wonderful work and feature Jacob!

@weaverryan weaverryan merged commit e26be66 into symfony:2.x Oct 12, 2023
10 checks passed
@jakubtobiasz jakubtobiasz deleted the gh-994/lazy-live-components branch October 12, 2023 20:09
weaverryan added a commit that referenced this pull request Oct 23, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LiveComponent] deferred/lazy loading
4 participants