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] Add variable to refer outer scope when rendering embedded components #936

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

sneakyvv
Copy link
Contributor

@sneakyvv sneakyvv commented Jun 11, 2023

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

This PR's is an alternative approach to the problem stated in #863.

Due to the changes proposed in #920, which deals with passing blocks down to nested embedded components, I realized that just being able to access the host (or parent) component isn't sufficient if blocks can be passed down multiple levels deep (as already mentioned in #863 (comment)).

This PR adds a proxy object Hierarchy which allows this syntax:

Component.php

final class Component
{
    public function scream(int $screamHowHard = 1): string
    {
        return 'sf-ux rules' . str_repeat('!', $screamHowHard);
    }
}
{# component.html.twig #}

<twig:Foo :someProp="this.something(1)">
    {{ someProp }}
    {{ outerScope.this.something(3) }}
</twig:Foo>
{# Foo.html.twig #}

<div>{% block content %}{% endblock %}</div>

Resulting in

<div>
    sf-ux rules!
    sf-ux rules!!!
</div>

Adding another layer below Foo, would ask for something like

{# component.html.twig #}

<twig:Foo>
    {{ outerScope.outerScope.this.something(3) }}
</twig:Foo>

to still point to Foo's function.


Questions

There's also a ComputedPropertiesProxy object. Should we have a cached version of the Hierarchy proxy?


NOTE

This PR continues on the changes of #920, because having a hierarchy only makes sense if blocks can be passed down multiple times. So #920 may have to be merged first, but if needed, and the outerScope.this syntax is preferred over the syntax of #863, I could push my branch that starts of the main branch instead.

Edited: using outerScope variable now instead Hierarchy proxy

@Kocal
Copy link
Member

Kocal commented Jun 20, 2023

IMO a child component should not have access to its parent.

@weaverryan
Copy link
Member

IMO a child component should not have access to its parent.

Well... the tricky thing is that you ARE still kind of "inside" the parent template - i.e. you're inside component.html.twig and want to access the Component object... but you currently can't if you're coding inside the <twig:Foo> section of that template.

I'm not on one side or the other yet.

@sneakyvv
Copy link
Contributor Author

Since #920 is merged now, I rebased this branch on the current HEAD.

@sneakyvv sneakyvv force-pushed the hierarchy branch 2 times, most recently from f32584d to 1b6f89d Compare August 30, 2023 08:36
@sneakyvv sneakyvv force-pushed the hierarchy branch 2 times, most recently from 6f5718c to d8ca2ef Compare September 5, 2023 20:46
{% component Alert with { type: 'success' } %}
{{ this.parent.someFunction }} {# this refers to SuccessAlert #}

{{ this.parent.someProp }} {# references a "someProp" prop from SuccessAlert #}
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this.parent (which requires us to use a wrapper Hierarchy object and magic methods), what about following our outerBlocks convention and adding an outerContext variable?

{{ outerContext.this.someProp }} 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you refer this.parent.parent then?

Copy link
Member

Choose a reason for hiding this comment

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

outerContext.outerContext.this.someProp? I think this would naturally occur if we started adding the outerContext variable, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed and using outerScope variable instead of outerContext as discussed via Slack.

@sneakyvv sneakyvv changed the title [TwigComponent] Add 'hierarchy' proxy to context when rendering embedded components [TwigComponent] Add variable to refer outer scope when rendering embedded components Sep 9, 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.

This is looking good - big set of tests 👍

{% set name = 'Fabien' %}
{% set message = 'Hello' %}
{% component Alert with { type: 'success', name: 'Bart' } %}
Hello {{ name }} {# name = Bart #}
Copy link
Member

Choose a reason for hiding this comment

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

It might be slightly more clear to just have:

Suggested change
Hello {{ name }} {# name = Bart #}
Hello {{ name }} {# Hello Bart #}

Especially in the next example, where name = Fabien isn't actually quite right. But showing the printed result, I think, is clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

{% component Alert with { type: 'success', name: 'Bart' } %}
Hello {{ name }} {# name = Bart #}

{{ message }} {{ outerScope.name }} {# message = 'Hello', name = Fabien #}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{{ message }} {{ outerScope.name }} {# message = 'Hello', name = Fabien #}
{{ message }} {{ outerScope.name }} {# Hello Fabien #}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

src/TwigComponent/doc/index.rst Show resolved Hide resolved
{% endcomponent %}

To keep the generic Alert component unaware of the ``someProp`` prop it is better to use ``outerScope.this``
instead of passing that info along via Alert's attributes.
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 delete this. It's true - but I think people might not understand what you mean, and will second guess themselves. Let's focus on showing them the tools they have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed


.. versionadded:: 2.12

The ability to refer to the scope of higher components via the ``outerScope`` variable was added in 2.12.
Copy link
Member

Choose a reason for hiding this comment

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

Move this to the top of the new docs

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

src/TwigComponent/src/ComponentRenderer.php Show resolved Hide resolved
@weaverryan
Copy link
Member

Thanks Bart!

@weaverryan weaverryan merged commit d993b12 into symfony:2.x Sep 27, 2023
9 checks passed
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.

3 participants