Compilation omits non-rendering code in extends
Opened this issue · 8 comments
Explain the problem
When using template inheritance, Melody will omit anything except for top-level variable assignments (using {% set .... %}
) that is not contained within a block.
Expected Behaviour
Melody should include all code that is non-rendering in its compilation.
Actual Behaviour
Melody only compiled top-level variable assignments.
Steps to reproduce
bar.html.twig
{% extends "foo.html.twig" %}
{% if bar %}
{% set foo = "hello" %}
{% endif %}
foo.html.twig
{{ foo | default('Ciao') }}!!
Rendering bar.html.twig
with bar
set to true
should emit hello!!
but will instead emit Ciao!!
.
Provide a repository that reproduces issue if possible
Provide your Environment details
- Node version:
any - Operating System:
any - melody version:
any
I would like to look into this. To start here is the repository that reproduces the issue https://github.com/danielcaldas/issue-non-rendering-code-in-extends
In discussion with @jamesb3ll we tought of two possible solutions for this issue.
For reference here is the pseudo output of compiling the given example.
foo.html.twig
_template.render = function (_context) {
text(_context.foo != null && _context.foo !== '' ? _context.foo : "Ciao");
};
bar.html.twig
_template.render = function (_context) {
let foo;
foo = "hello";
_parent.render.call(_template, _context); // render parent foo.html.twig
};
Solution 1
foo
is in fact expected on the _context
when rendering the parent so instead of only declaring the variable in the bar.html.twig
we could also write this variable in the _context
.
Solution 2
Since render is executed with the _template
context we could assign the foo
to the _template
scope and access it via this.foo
on the parent render
function.
Do you think these are valid approaches? Can you think of a better one for this? I would like to hear your opinion @pago
Problem with Solution 2, how will foo.html.twig
know to access foo
from this
instead of _context
. Writing to _context
seems better to me, but there is probably a solution we haven't thought of? Or any side-effects that writing to _context
could have?
We don't use this
as far as I remember so solution 1 would be more appropriate.
But please make sure that you don't actually mutate _context
. Instead, you'll need to create a new child context, mutate that and pass it to the parent. For that specific case, we should already be able to do it if you adapt the inference for the "local usage only" optimization.
If you'd add a {% include "foo.twig" %}
Melody would do the set
correctly. So the inference should treat inheritance as if it was an include by disabling the optimization.
The original bug report is also about Melody dropping if
and similar statements in inherited templates. Will you have a look at that as well?
Ok we agree that solution 1 is more appropriate considering the things that @pago added regarding _context
mutation.
Regarding the drop of if
and similar statements.. So are we thinking of removing those statements and throw some error when a user tries to use them on inherited templates? I'm a bit confused because at the moment twig allows the use of those statements inside block
s, would we be removing that capability as well?
Sorry for the confusion. Let me clarify:
Given the template above:
{% extends "foo.html.twig" %}
{% if bar %}
{% set foo = "hello" %}
{% endif %}
Melody will currently emit something like what you provided:
_template.render = function (_context) {
let foo;
foo = "hello";
_parent.render.call(_template, _context); // render parent foo.html.twig
};
Problem 1 is not passing the new value of foo
to the parent, however, problem 2 is that we lost the if
condition (if bar
).
Fixing one without the other will only result in more issues. :)
@danielcaldas I can't remember but I think we didn't address this yet, right?
Hey @pago no I didn't finalize it, there is some work in progress here https://github.com/danielcaldas/melody/tree/fix/omitted-code-twig-extends, but it's been a while.