From e566d8ecbb7016cf3a8861a47a4a52e4a503b838 Mon Sep 17 00:00:00 2001 From: Juro Oravec Date: Thu, 2 May 2024 22:24:49 +0200 Subject: [PATCH] fix: loader tags compatibility (#468) --- docs/slots_and_blocks.md | 166 ++++++++++ src/django_components/component.py | 8 + src/django_components/node.py | 61 +++- src/django_components/slots.py | 12 +- .../templates/block_in_slot_in_component.html | 17 + tests/templates/block_inside_component.html | 13 + .../extendable_template_with_blocks.html | 18 +- tests/templates/slot_inside_block.html | 17 + tests/templates/slot_inside_extends.html | 1 + tests/templates/slot_inside_include.html | 1 + tests/test_templatetags.py | 300 +++++++++++++++++- 11 files changed, 592 insertions(+), 22 deletions(-) create mode 100644 docs/slots_and_blocks.md create mode 100644 tests/templates/block_in_slot_in_component.html create mode 100644 tests/templates/block_inside_component.html create mode 100644 tests/templates/slot_inside_block.html create mode 100644 tests/templates/slot_inside_extends.html create mode 100644 tests/templates/slot_inside_include.html diff --git a/docs/slots_and_blocks.md b/docs/slots_and_blocks.md new file mode 100644 index 00000000..53102ca4 --- /dev/null +++ b/docs/slots_and_blocks.md @@ -0,0 +1,166 @@ +# Using `slot` and `block` tags + +1. First let's clarify how `include` and `extends` tags work inside components. + So when component template includes `include` or `extends` tags, it's as if the "included" + template was inlined. So if the "included" template contains `slot` tags, then the component + uses those slots. + + So if you have a template `abc.html`: + ```django +
+ hello + {% slot "body" %}{% endslot %} +
+ ``` + + And components that make use of `abc.html` via `include` or `extends`: + ```py + @component.register("my_comp_extends") + class MyCompWithExtends(component.Component): + template = """{% extends "abc.html" %}""" + + @component.register("my_comp_include") + class MyCompWithInclude(component.Component): + template = """{% include "abc.html" %}""" + ``` + + Then you can set slot fill for the slot imported via `include/extends`: + + ```django + {% component "my_comp_extends" %} + {% fill "body" %} + 123 + {% endfill %} + {% endcomponent %} + ``` + + And it will render: + ```html +
+ hello + 123 +
+ ``` + +2. Slot and block + + So if you have a template `abc.html` like so: + + ```django +
+ hello + {% block inner %} + 1 + {% slot "body" %} + 2 + {% endslot %} + {% endblock %} +
+ ``` + + and component `my_comp`: + + ```py + @component.register("my_comp") + class MyComp(component.Component): + template_name = "abc.html" + ``` + + Then: + + 1. Since the `block` wasn't overriden, you can use the `body` slot: + + ```django + {% component "my_comp" %} + {% fill "body" %} + XYZ + {% endfill %} + {% endcomponent %} + ``` + + And we get: + + ```html +
hello 1 XYZ
+ ``` + + 2. `blocks` CANNOT be overriden through the `component` tag, so something like this: + + ```django + {% component "my_comp" %} + {% fill "body" %} + XYZ + {% endfill %} + {% endcomponent %} + {% block "inner" %} + 456 + {% endblock %} + ``` + + Will still render the component content just the same: + + ```html +
hello 1 XYZ
+ ``` + + 3. You CAN override the `block` tags of `abc.html` if my component template + uses `extends`. In that case, just as you would expect, the `block inner` inside + `abc.html` will render `OVERRIDEN`: + + ````py + @component.register("my_comp") + class MyComp(component.Component): + template_name = """ + {% extends "abc.html" %} + + {% block inner %} + OVERRIDEN + {% endblock %} + """ + ``` + + ```` + + 4. This is where it gets interesting (but still intuitive). You can insert even + new `slots` inside these "overriding" blocks: + + ```py + @component.register("my_comp") + class MyComp(component.Component): + template_name = """ + {% extends "abc.html" %} + + {% load component_tags %} + {% block "inner" %} + OVERRIDEN + {% slot "new_slot" %} + hello + {% endslot %} + {% endblock %} + """ + ``` + + And you can then pass fill for this `new_slot` when rendering the component: + + ```django + {% component "my_comp" %} + {% fill "new_slot" %} + XYZ + {% endfill %} + {% endcomponent %} + ``` + + NOTE: Currently you can supply fills for both `new_slot` and `body` slots, and you will + not get an error for an invalid/unknown slot name. But since `body` slot is not rendered, + it just won't do anything. So this renders the same as above: + + ```django + {% component "my_comp" %} + {% fill "new_slot" %} + XYZ + {% endfill %} + {% fill "body" %} + www + {% endfill %} + {% endcomponent %} + ``` diff --git a/src/django_components/component.py b/src/django_components/component.py index c5db0b27..5e4abaf5 100644 --- a/src/django_components/component.py +++ b/src/django_components/component.py @@ -9,6 +9,7 @@ from django.forms.widgets import Media, MediaDefiningClass from django.http import HttpResponse from django.template.base import FilterExpression, Node, NodeList, Template, TextNode from django.template.context import Context +from django.template.exceptions import TemplateSyntaxError from django.template.loader import get_template from django.utils.html import escape from django.utils.safestring import SafeString, mark_safe @@ -291,6 +292,7 @@ class Component(View, metaclass=SimplifiedInterfaceMediaDefiningClass): context_data = {} slots, resolved_fills = resolve_slots( + context, template, component_name=self.registered_name, context_data=context_data, @@ -402,6 +404,12 @@ class ComponentNode(Node): # Note that outer component context is used to resolve variables in # fill tag. resolved_name = fill_node.name_fexp.resolve(context) + if resolved_name in fill_content: + raise TemplateSyntaxError( + f"Multiple fill tags cannot target the same slot name: " + f"Detected duplicate fill tag name '{resolved_name}'." + ) + resolved_fill_alias = fill_node.resolve_alias(context, resolved_component_name) fill_content[resolved_name] = FillContent(fill_node.nodelist, resolved_fill_alias) diff --git a/src/django_components/node.py b/src/django_components/node.py index a89dfd5a..7d77bfa6 100644 --- a/src/django_components/node.py +++ b/src/django_components/node.py @@ -1,7 +1,9 @@ from typing import Callable, List, NamedTuple, Optional +from django.template import Context, Template from django.template.base import Node, NodeList, TextNode from django.template.defaulttags import CommentNode +from django.template.loader_tags import ExtendsNode, IncludeNode, construct_relative_path def nodelist_has_content(nodelist: NodeList) -> bool: @@ -20,26 +22,79 @@ class NodeTraverse(NamedTuple): parent: Optional["NodeTraverse"] -def walk_nodelist(nodes: NodeList, callback: Callable[[Node], Optional[str]]) -> None: +def walk_nodelist( + nodes: NodeList, + callback: Callable[[Node], Optional[str]], + context: Optional[Context] = None, +) -> None: """Recursively walk a NodeList, calling `callback` for each Node.""" node_queue: List[NodeTraverse] = [NodeTraverse(node=node, parent=None) for node in nodes] while len(node_queue): traverse = node_queue.pop() callback(traverse) - child_nodes = get_node_children(traverse.node) + child_nodes = get_node_children(traverse.node, context) child_traverses = [NodeTraverse(node=child_node, parent=traverse) for child_node in child_nodes] node_queue.extend(child_traverses) -def get_node_children(node: Node) -> NodeList: +def get_node_children(node: Node, context: Optional[Context] = None) -> NodeList: """ Get child Nodes from Node's nodelist atribute. This function is taken from `get_nodes_by_type` method of `django.template.base.Node`. """ + # Special case - {% extends %} tag - Load the template and go deeper + if isinstance(node, ExtendsNode): + # NOTE: When {% extends %} node is being parsed, it collects all remaining template + # under node.nodelist. + # Hence, when we come across ExtendsNode in the template, we: + # 1. Go over all nodes in the template using `node.nodelist` + # 2. Go over all nodes in the "parent" template, via `node.get_parent` + nodes = NodeList() + nodes.extend(node.nodelist) + template = node.get_parent(context) + nodes.extend(template.nodelist) + return nodes + + # Special case - {% include %} tag - Load the template and go deeper + elif isinstance(node, IncludeNode): + template = get_template_for_include_node(node, context) + return template.nodelist + nodes = NodeList() for attr in node.child_nodelists: nodelist = getattr(node, attr, []) if nodelist: nodes.extend(nodelist) return nodes + + +def get_template_for_include_node(include_node: IncludeNode, context: Context) -> Template: + """ + This snippet is taken directly from `IncludeNode.render()`. Unfortunately the + render logic doesn't separate out template loading logic from rendering, so we + have to copy the method. + """ + template = include_node.template.resolve(context) + # Does this quack like a Template? + if not callable(getattr(template, "render", None)): + # If not, try the cache and select_template(). + template_name = template or () + if isinstance(template_name, str): + template_name = ( + construct_relative_path( + include_node.origin.template_name, + template_name, + ), + ) + else: + template_name = tuple(template_name) + cache = context.render_context.dicts[0].setdefault(include_node, {}) + template = cache.get(template_name) + if template is None: + template = context.template.engine.select_template(template_name) + cache[template_name] = template + # Use the base.Template of a backends.django.Template. + elif hasattr(template, "template"): + template = template.template + return template diff --git a/src/django_components/slots.py b/src/django_components/slots.py index 9b3bd136..874e8a67 100644 --- a/src/django_components/slots.py +++ b/src/django_components/slots.py @@ -258,15 +258,18 @@ def _try_parse_as_named_fill_tag_set( ComponentNodeCls: Type[Node], ) -> List[FillNode]: result = [] - seen_name_fexps: Set[FilterExpression] = set() + seen_name_fexps: Set[str] = set() for node in nodelist: if isinstance(node, FillNode): - if node.name_fexp in seen_name_fexps: + # Check that, after we've resolved the names, that there's still no duplicates. + # This makes sure that if two different variables refer to same string, we detect + # them. + if node.name_fexp.token in seen_name_fexps: raise TemplateSyntaxError( f"Multiple fill tags cannot target the same slot name: " f"Detected duplicate fill tag name '{node.name_fexp}'." ) - seen_name_fexps.add(node.name_fexp) + seen_name_fexps.add(node.name_fexp.token) result.append(node) elif isinstance(node, CommentNode): pass @@ -308,6 +311,7 @@ def _try_parse_as_default_fill( def resolve_slots( + context: Context, template: Template, component_name: Optional[str], context_data: Dict[str, Any], @@ -374,7 +378,7 @@ def resolve_slots( slot_children[parent_slot_id].append(node.node_id) break - walk_nodelist(template.nodelist, on_node) + walk_nodelist(template.nodelist, on_node, context) # 3. Figure out which slot the default/implicit fill belongs to slot_fills = _resolve_default_slot( diff --git a/tests/templates/block_in_slot_in_component.html b/tests/templates/block_in_slot_in_component.html new file mode 100644 index 00000000..3c264f17 --- /dev/null +++ b/tests/templates/block_in_slot_in_component.html @@ -0,0 +1,17 @@ +{% load component_tags %} + + + + {% component "slotted_component" %} + {% fill "header" %}{% endfill %} + {% fill "main" %} + {% slot "body" %} + Helloodiddoo + {% block inner %} + Default inner + {% endblock %} + {% endslot %} + {% endfill %} + {% endcomponent %} + + diff --git a/tests/templates/block_inside_component.html b/tests/templates/block_inside_component.html new file mode 100644 index 00000000..c70504ba --- /dev/null +++ b/tests/templates/block_inside_component.html @@ -0,0 +1,13 @@ +{% load component_tags %} + + + + {% component "slotted_component" %} + {% fill "header" %}{% endfill %} + {% fill "main" %} + {% block body %} + {% endblock %} + {% endfill %} + {% endcomponent %} + + diff --git a/tests/templates/extendable_template_with_blocks.html b/tests/templates/extendable_template_with_blocks.html index 9cbfb9ee..01a2d3c3 100644 --- a/tests/templates/extendable_template_with_blocks.html +++ b/tests/templates/extendable_template_with_blocks.html @@ -1,12 +1,12 @@ {% load component_tags %} - -
-
- {% block body %} - {% endblock %} -
-
- - \ No newline at end of file + +
+
+ {% block body %} + {% endblock %} +
+
+ + diff --git a/tests/templates/slot_inside_block.html b/tests/templates/slot_inside_block.html new file mode 100644 index 00000000..b914251d --- /dev/null +++ b/tests/templates/slot_inside_block.html @@ -0,0 +1,17 @@ +{% load component_tags %} + + + + {% component "slotted_component" %} + {% fill "header" %}{% endfill %} + {% fill "main" %} + Helloodiddoo + {% block inner %} + {% slot "body" %} + Default inner + {% endslot %} + {% endblock %} + {% endfill %} + {% endcomponent %} + + diff --git a/tests/templates/slot_inside_extends.html b/tests/templates/slot_inside_extends.html new file mode 100644 index 00000000..58ea6d14 --- /dev/null +++ b/tests/templates/slot_inside_extends.html @@ -0,0 +1 @@ +{% extends "block_in_slot_in_component.html" %} diff --git a/tests/templates/slot_inside_include.html b/tests/templates/slot_inside_include.html new file mode 100644 index 00000000..31e030df --- /dev/null +++ b/tests/templates/slot_inside_include.html @@ -0,0 +1 @@ +{% include "block_in_slot_in_component.html" %} diff --git a/tests/test_templatetags.py b/tests/test_templatetags.py index 40dffc25..8531de8e 100644 --- a/tests/test_templatetags.py +++ b/tests/test_templatetags.py @@ -114,6 +114,23 @@ class _ComplexParentComponent(component.Component): return {"items": items} +class BlockInSlotInComponent(component.Component): + template_name = "block_in_slot_in_component.html" + + +class SlotInsideExtendsComponent(component.Component): + template_name = "slot_inside_extends.html" + + +class SlotInsideIncludeComponent(component.Component): + template_name = "slot_inside_include.html" + + +####################### +# TESTS +####################### + + class ComponentTemplateTagTest(BaseTestCase): def setUp(self): # NOTE: component.registry is global, so need to clear before each test @@ -1045,7 +1062,7 @@ class TemplateSyntaxErrorTests(BaseTestCase): ).render(Context({})) def test_isolated_slot_is_error(self): - with self.assertRaises(TemplateSyntaxError): + with self.assertRaises(KeyError): Template( """ {% load component_tags %} @@ -1062,13 +1079,27 @@ class TemplateSyntaxErrorTests(BaseTestCase): Template( """ {% load component_tags %} - {% component "broken_component" %} + {% component "test" %} {% fill "header" %}Custom header {% endfill %} {% fill "header" %}Other header{% endfill %} {% endcomponent %} """ ).render(Context({})) + def test_non_unique_fill_names_is_error_via_vars(self): + with self.assertRaises(TemplateSyntaxError): + Template( + """ + {% load component_tags %} + {% with var1="header" var2="header" %} + {% component "test" %} + {% fill var1 %}Custom header {% endfill %} + {% fill var2 %}Other header{% endfill %} + {% endcomponent %} + {% endwith %} + """ + ).render(Context({})) + class ComponentNestingTests(BaseTestCase): @classmethod @@ -1422,9 +1453,7 @@ class ContextVarsTests(BaseTestCase): self.assertHTMLEqual(rendered, expected) -class RegressionTests(BaseTestCase): - """Ensure we don't break the same thing AGAIN.""" - +class BlockCompatTests(BaseTestCase): def setUp(self): component.registry.clear() super().setUp() @@ -1434,7 +1463,61 @@ class RegressionTests(BaseTestCase): super().tearDownClass() component.registry.clear() - def test_block_and_extends_tag_works(self): + def test_slots_inside_extends(self): + component.registry.register("slotted_component", SlottedComponent) + component.registry.register("slot_inside_extends", SlotInsideExtendsComponent) + + template = """ + {% load component_tags %} + {% component "slot_inside_extends" %} + {% fill "body" %} + BODY_FROM_FILL + {% endfill %} + {% endcomponent %} + """ + rendered = Template(template).render(Context()) + expected = """ + + + + +
+
BODY_FROM_FILL
+
Default footer
+ + + + """ + self.assertHTMLEqual(rendered, expected) + + def test_slots_inside_include(self): + component.registry.register("slotted_component", SlottedComponent) + component.registry.register("slot_inside_include", SlotInsideIncludeComponent) + + template = """ + {% load component_tags %} + {% component "slot_inside_include" %} + {% fill "body" %} + BODY_FROM_FILL + {% endfill %} + {% endcomponent %} + """ + rendered = Template(template).render(Context()) + expected = """ + + + + +
+
BODY_FROM_FILL
+
Default footer
+ + + + """ + self.assertHTMLEqual(rendered, expected) + + def test_component_inside_block(self): component.registry.register("slotted_component", SlottedComponent) template = """ {% extends "extendable_template_with_blocks.html" %} @@ -1468,6 +1551,211 @@ class RegressionTests(BaseTestCase): """ self.assertHTMLEqual(rendered, expected) + def test_block_inside_component(self): + component.registry.register("slotted_component", SlottedComponent) + + template = """ + {% extends "block_inside_component.html" %} + {% load component_tags %} + {% block body %} +
+ 58 giraffes and 2 pantaloons +
+ {% endblock %} + """ + rendered = Template(template).render(Context()) + expected = """ + + + + +
+
+
58 giraffes and 2 pantaloons
+
+
Default footer
+ + + + """ + self.assertHTMLEqual(rendered, expected) + + def test_block_does_not_affect_inside_component(self): + """ + Assert that when we call a component with `{% component %}`, that + the `{% block %}` will NOT affect the inner component. + """ + component.registry.register("slotted_component", SlottedComponent) + component.registry.register("block_inside_slot_v1", BlockInSlotInComponent) + + template = """ + {% load component_tags %} + {% component "block_inside_slot_v1" %} + {% fill "body" %} + BODY_FROM_FILL + {% endfill %} + {% endcomponent %} + {% block inner %} + wow + {% endblock %} + """ + rendered = Template(template).render(Context()) + expected = """ + + + + +
+
BODY_FROM_FILL
+
Default footer
+ + + + wow + """ + self.assertHTMLEqual(rendered, expected) + + def test_slot_inside_block__slot_default_block_default(self): + component.registry.register("slotted_component", SlottedComponent) + + @component.register("slot_inside_block") + class _SlotInsideBlockComponent(component.Component): + template = """{% extends "slot_inside_block.html" %}""" + + template = """ + {% load component_tags %} + {% component "slot_inside_block" %}{% endcomponent %} + """ + rendered = Template(template).render(Context()) + expected = """ + + + + +
+
+ Helloodiddoo + Default inner +
+
Default footer
+ + + + """ + self.assertHTMLEqual(rendered, expected) + + def test_slot_inside_block__slot_default_block_override(self): + component.registry.register("slotted_component", SlottedComponent) + + @component.register("slot_inside_block") + class _SlotInsideBlockComponent(component.Component): + template = """ + {% extends "slot_inside_block.html" %} + {% block inner %} + INNER BLOCK OVERRIDEN + {% endblock %} + """ + + template = """ + {% load component_tags %} + {% component "slot_inside_block" %}{% endcomponent %} + """ + rendered = Template(template).render(Context()) + expected = """ + + + + +
+
+ Helloodiddoo + INNER BLOCK OVERRIDEN +
+
Default footer
+ + + + """ + self.assertHTMLEqual(rendered, expected) + + def test_slot_inside_block__slot_overriden_block_default(self): + component.registry.register("slotted_component", SlottedComponent) + + @component.register("slot_inside_block") + class _SlotInsideBlockComponent(component.Component): + template = """{% extends "slot_inside_block.html" %}""" + + template = """ + {% load component_tags %} + {% component "slot_inside_block" %} + {% fill "body" %} + SLOT OVERRIDEN + {% endfill %} + {% endcomponent %} + """ + rendered = Template(template).render(Context()) + expected = """ + + + + +
+
+ Helloodiddoo + SLOT OVERRIDEN +
+
Default footer
+ + + + """ + self.assertHTMLEqual(rendered, expected) + + def test_slot_inside_block__slot_overriden_block_overriden(self): + component.registry.register("slotted_component", SlottedComponent) + + @component.register("slot_inside_block") + class _SlotInsideBlockComponent(component.Component): + template = """ + {% extends "slot_inside_block.html" %} + {% block inner %} + {% load component_tags %} + {% slot "new_slot" %}{% endslot %} + {% endblock %} + whut + """ + + # NOTE: The "body" fill will NOT show up, because we override the `inner` block + # with a different slot. But the "new_slot" WILL show up. + template = """ + {% load component_tags %} + {% component "slot_inside_block" %} + {% fill "body" %} + SLOT_BODY__OVERRIDEN + {% endfill %} + {% fill "new_slot" %} + SLOT_NEW__OVERRIDEN + {% endfill %} + {% endcomponent %} + """ + rendered = Template(template).render(Context()) + expected = """ + + + + +
+
+ Helloodiddoo + SLOT_NEW__OVERRIDEN +
+ + + + + """ + self.assertHTMLEqual(rendered, expected) + class IterationFillTest(BaseTestCase): """Tests a behaviour of {% fill .. %} tag which is inside a template {% for .. %} loop."""