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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TwigComponent] [LiveComponent] Add support for embedded live components #913

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

sneakyvv
Copy link
Contributor

@sneakyvv sneakyvv commented May 30, 2023

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

Context

Using embedded components was introduced in #317, but support for Live embedded components was not added. The issue is that on a re-render of a Live component you lose the blocks defined within an embedded component. This PR solves that issue.

Example

To explain the solution, take this example:

{# someTemplate.html.twig #}
{% component Foo %}
  {% block content %}
    Override content
  {% endblock %}
{% endcomponent %}
{# Foo.html.twig #}
<div {{ attributes }}>
  {% block content %}
    Default content
  {% endblock %}
</div>

Of course, Foo is a Live component.

This obviously also works with the new Twig syntax.

Background

  1. Each {% component %} tag is compiled by ComponentNode.
    It adds an embedded Template to the Template class compiled for someTemplate.html.twig. This is a second class inside the same php file, with a suffix in the form of ___%d. That number at the end is normally random, and is called the embedded template index.
  2. ComponentNode would generate Template code which fetches the embeddedContext from the ComponentRenderer and passed that to the loadTemplate('Foo.html.twig', $index)->display()
  3. When a component is re-rendered (via an action callback) it uses the template of the component (Foo.html.twig), which does not have the original block content, because that's part of the host Template (someTemplate.html.twig).

Solution

We only need to use the embedded Template instead of the component Template to re-render a component.

To make this happen, we need to:

  1. Use a deterministic index for an embedded template during compilation.
  2. Store info on the rendered component's HTML (via the attributes) about the host template and the embedded template's index.
  3. Load the embedded Template during re-render using the info passed along with the other attributes/props.

Remaining

  1. I use loadTemplate now in the ComponentRender, which is marked as internal in the Twig package. Can we ignore that within this package (as both are "Symfony")?
  2. The PreRenderEvent::EMBEDDED constant and the isEmbedded function were introduced to block live embedded components. Should this PR remove those as well?

Tasks

  • Remove isEmbedded?
  • Remove PreRenderEvent::EMBEDDED?
  • Add CHANGELOG

@sneakyvv sneakyvv changed the title Add support for embedded live components [TwigComponent] [LiveComponent] Add support for embedded live components May 30, 2023
@sneakyvv sneakyvv force-pushed the embedded-live-components branch 5 times, most recently from 8333edc to b158278 Compare May 31, 2023 20:03
@sneakyvv
Copy link
Contributor Author

@kbond FYI: had some weird issues with the tests for PHP 8.0 (but ultimately stupid mistake on my part) which are now fixed. Everything is green so I'd love to hear your feedback!

@sneakyvv
Copy link
Contributor Author

Rebased my branch + removed an isChildComponent check in the LiveControllerAttributesCreator so any component gets a live-id (as discussed with @weaverryan).
This is needed now so embedded components also get a live-id, otherwise actions to nested components wouldn't be linked to the embedded component (but some higher non-embedded component).

It used to be that

  1. only non-embedded components would get a live-id, since
    • only those are rendered via ComponentRenderer::render(),
    • and only that function uses the ComponentStack,
    • by which the isChildComponent is determined
  2. while embedded components are rendered via ComponentRenderer::embeddedContext() + Template::display().

Since embedded components couldn't be live, it follows that live components would always have a live-id (since all of them were guaranteed non-embedded and follow flow 1).
Ergo, now we have to add live-ids on embedded (live) components as well.

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.

Just a quick first glance to try to find problems. This is pretty amazing

dependency-versions: lowest
# needed for php 8.0 to skip symfony/stimulus-bundle
composer-options: "--ignore-platform-reqs"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can all likely be reverted now - the lowest php version and test matrix have all been updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}
if ($mounted->hasExtraMetadata('hostTemplate') && $mounted->hasExtraMetadata('embeddedTemplateIndex')) {
$mountedAttributes = $mountedAttributes->defaults([
'data-host-template' => $mounted->getExtraMetadata('hostTemplate'),
Copy link
Member

@weaverryan weaverryan Jun 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exposes the raw Twig template name to the frontend, right? I totally get why, but I don't LOVE it. And I also can't think of a GREAT way around it. We could... encrypt it using the secret? Or... in theory could we, during cache warmup, use the twig.template_iterator service to create a "map" of "random string => template name"?

Edit: I chatted with kbond - he seems to like the cache warmup idea, if we can pull it off :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed via Slack, I wondered if this was a big deal, but as agreed that it's just to be on the safe side, I'll look into the cache warmup option.

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 added the suggested cache warmer, and obscured the host template attribute through the resulting map.

@sneakyvv
Copy link
Contributor Author

I noticed that live (now embedded) components would always re-render, unlike non-embedded components which would only re-render if a LiveProp changed that also allowed the parent component to update the child component. If none of those LiveProps changed no re-render would happen as of 2.8.

This is because of the InterceptChildComponentRenderSubscriber acting on the PreCreateForRenderEvent and short-circuiting the render process, returning a fake/incomplete html for a child component. It's the frontend that decides whether the props have changed and whether another callback to the backend is desired.

However, because non-embedded components are rendered via the createAndRender function, while embedded components are rendered via ComponentRenderer::embeddedContext() + loadTemplate(), the latter don't get a chance to be short-circuited, and have their non-updateable properties be removed, resulting in skipping an redundant and perhaps even unwanted re-render.

I added a new commit to take care of this.

@sneakyvv
Copy link
Contributor Author

sneakyvv commented Jul 1, 2023

I added a change to make embedded components also be part of the component stack, by which it is determined to add a fingerprint.
Previously, I just removed the isChildComponent-check in the LiveControllerAttributesCreator but after discussing this with @weaverryan, who told me it can be CPU-heavy to generate fingerprints, I decided to handle it this way, by also adding embedded components to the component stack. This way at least root components don't get an unnecessary fingerprint.

@sneakyvv
Copy link
Contributor Author

sneakyvv commented Aug 8, 2023

@kbond @weaverryan can you guys have another look at this PR?

public function createAndRender(string $name, array $props = []): string
/**
* Allow the render process to be short-circuited.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dispatch an event before rendering to give an opportunity to short-circuit the rendering process.

@weaverryan
Copy link
Member

This is highly technical, but WOW! I can't believe you got this working. I played with it locally, and it's a ton of fun. Great work Bart! Now, time for some docs about this - we need to be sure to mention the limitation of "relying" on an outside variable from within the embedded template contents.

@weaverryan weaverryan merged commit 71aebf1 into symfony:2.x Aug 18, 2023
33 of 38 checks passed
@sneakyvv sneakyvv deleted the embedded-live-components branch August 23, 2023 09:07
weaverryan added a commit that referenced this pull request Aug 29, 2023
…ts doc (sneakyvv)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[TwigComponent][LiveComponent] Add Embedded Live Components doc

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| Tickets       |
| License       | MIT

Adds documentation for the feature added by #913

Commits
-------

a62c813 [TwigComponent][LiveComponent] Add Embedded Live Components doc
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.

2 participants