From a5bd0cf2e3bffe7a12b326cdfd323fe7a5b08d96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emil=20Stenstr=C3=B6m?= Date: Sun, 30 May 2021 18:46:11 +0200 Subject: [PATCH] Rework slot management to avoid nodelist copying (fixes #64) Co-authored-by: rbeard0330 <@dul2k3BKW6m> --- django_components/component.py | 40 ++++--------------- .../templatetags/component_tags.py | 20 ++++++---- .../templates/template_with_illegal_slot.html | 2 + tests/test_templatetags.py | 30 +++++++++++++- 4 files changed, 52 insertions(+), 40 deletions(-) create mode 100644 tests/templates/template_with_illegal_slot.html diff --git a/django_components/component.py b/django_components/component.py index 1fd9d2c5..18e22012 100644 --- a/django_components/component.py +++ b/django_components/component.py @@ -1,10 +1,9 @@ import warnings -from copy import copy, deepcopy from functools import lru_cache from django.conf import settings from django.forms.widgets import MediaDefiningClass -from django.template.base import Node, NodeList, TokenType +from django.template.base import Node, TokenType from django.template.loader import get_template from django.utils.safestring import mark_safe @@ -12,6 +11,7 @@ from django.utils.safestring import mark_safe from django_components.component_registry import AlreadyRegistered, ComponentRegistry, NotRegistered # noqa TEMPLATE_CACHE_SIZE = getattr(settings, "COMPONENTS", {}).get('TEMPLATE_CACHE_SIZE', 128) +ACTIVE_SLOT_CONTEXT_KEY = '_DJANGO_COMPONENTS_ACTIVE_SLOTS' class SimplifiedInterfaceMediaDefiningClass(MediaDefiningClass): @@ -80,18 +80,11 @@ class Component(metaclass=SimplifiedInterfaceMediaDefiningClass): @lru_cache(maxsize=TEMPLATE_CACHE_SIZE) def get_processed_template(self, template_name): - """Retrieve the requested template and add a link to this component to each SlotNode in the template.""" + """Retrieve the requested template and check for unused slots.""" - source_template = get_template(template_name) + component_template = get_template(template_name).template - # The template may be shared with another component (e.g., due to caching). To ensure that each - # SlotNode is unique between components, we have to copy the nodes in the template nodelist and - # any contained nodelists. - component_template = copy(source_template.template) - cloned_nodelist = [duplicate_node(node) for node in component_template.nodelist] - component_template.nodelist = NodeList(cloned_nodelist) - - # Traverse template nodes and descendants, and give each slot node a reference to this component. + # Traverse template nodes and descendants visited_nodes = set() nodes_to_visit = list(component_template.nodelist) slots_seen = set() @@ -104,7 +97,6 @@ class Component(metaclass=SimplifiedInterfaceMediaDefiningClass): nodes_to_visit.extend(getattr(current_node, nodelist_name, [])) if self.is_slot_node(current_node): slots_seen.add(current_node.name) - current_node.parent_component = self # Check and warn for unknown slots if settings.DEBUG: @@ -122,7 +114,9 @@ class Component(metaclass=SimplifiedInterfaceMediaDefiningClass): def render(self, context): template_name = self.template(context) instance_template = self.get_processed_template(template_name) - return instance_template.render(context) + active_slots = {**context.get(ACTIVE_SLOT_CONTEXT_KEY, {}), **self.slots} + with context.update({ACTIVE_SLOT_CONTEXT_KEY: active_slots}): + return instance_template.render(context) class Media: css = {} @@ -133,24 +127,6 @@ class Component(metaclass=SimplifiedInterfaceMediaDefiningClass): registry = ComponentRegistry() -def duplicate_node(source_node): - """Perform a shallow copy of source_node and then recursively copy over each of source_node's nodelists. - - If a nodelist is a dynamic property that cannot be set, fall back to a deepcopy of source_node.""" - - try: - clone = copy(source_node) - for nodelist_name in source_node.child_nodelists: - nodelist = getattr(source_node, nodelist_name, NodeList()) - nodelist_contents = [duplicate_node(n) for n in nodelist] - setattr(clone, nodelist_name, type(nodelist)(nodelist_contents)) - return clone - except AttributeError: - # AttributeError is raised if an attribute cannot be set (e.g., IfNode's nodelist, - # which is a read-only property). - return deepcopy(source_node) - - def register(name): """Class decorator to register a component. diff --git a/django_components/templatetags/component_tags.py b/django_components/templatetags/component_tags.py index 68b0d0f8..585941ae 100644 --- a/django_components/templatetags/component_tags.py +++ b/django_components/templatetags/component_tags.py @@ -1,3 +1,4 @@ +import inspect from collections import defaultdict from django import template @@ -6,13 +7,11 @@ from django.template.base import Node, NodeList, TemplateSyntaxError, TokenType from django.template.library import parse_bits from django.utils.safestring import mark_safe -from django_components.component import registry +from django_components.component import ACTIVE_SLOT_CONTEXT_KEY, Component, registry from django_components.middleware import CSS_DEPENDENCY_PLACEHOLDER, JS_DEPENDENCY_PLACEHOLDER register = template.Library() -SLOT_CONTEXT_KEY = "__slot_context" - def get_components_from_registry(registry): """Returns a list unique components from the registry.""" @@ -94,9 +93,17 @@ class SlotNode(Node): cloned_node.parent_component = self.parent_component cloned_node.context = context - assert not NodeList(), "Logic in SlotNode.render method assumes that empty nodelists are falsy." with context.update({'slot': cloned_node}): - return cloned_node.parent_component.slots.get(cloned_node.name, cloned_node.nodelist).render(context) + return self.get_nodelist(context).render(context) + + def get_nodelist(self, context): + if ACTIVE_SLOT_CONTEXT_KEY not in context: + raise TemplateSyntaxError(f'Attempted to render SlotNode {self.name} outside of a parent Component or ' + 'without access to context provided by its parent Component. This will not' + 'work properly.') + + overriding_nodelist = context[ACTIVE_SLOT_CONTEXT_KEY].get(self.name, None) + return overriding_nodelist if overriding_nodelist is not None else self.nodelist def super(self): """Render default slot content.""" @@ -150,7 +157,6 @@ class ComponentNode(Node): context = context.new() with context.update(component_context): - context.render_context[SLOT_CONTEXT_KEY] = {} rendered_component = self.component.render(context) if self.should_render_dependencies: return f'' + rendered_component @@ -192,7 +198,7 @@ def slot_tokens(parser): def is_whitespace(token): return token.token_type == TokenType.TEXT and not token.contents.strip() - def is_block_tag(token, /, name): + def is_block_tag(token, name): return token.token_type == TokenType.BLOCK and token.split_contents()[0] == name while True: diff --git a/tests/templates/template_with_illegal_slot.html b/tests/templates/template_with_illegal_slot.html new file mode 100644 index 00000000..370f217a --- /dev/null +++ b/tests/templates/template_with_illegal_slot.html @@ -0,0 +1,2 @@ +{% load component_tags %} +{% include 'slotted_template.html' with context=None only %} diff --git a/tests/test_templatetags.py b/tests/test_templatetags.py index c3d96864..ceff565c 100644 --- a/tests/test_templatetags.py +++ b/tests/test_templatetags.py @@ -2,9 +2,9 @@ from textwrap import dedent from django.template import Context, Template, TemplateSyntaxError -from .django_test_setup import * # NOQA from django_components import component +from .django_test_setup import * # NOQA from .testutils import Django30CompatibleSimpleTestCase as SimpleTestCase @@ -33,6 +33,11 @@ class SlottedComponent(component.Component): return "slotted_template.html" +class BrokenComponent(component.Component): + def template(self, context): + return "template_with_illegal_slot.html" + + class SlottedComponentWithMissingVariable(component.Component): def template(self, context): return "slotted_template_with_missing_variable.html" @@ -589,6 +594,7 @@ class TemplateSyntaxErrorTests(SimpleTestCase): def setUpClass(cls): super().setUpClass() component.registry.register('test', SlottedComponent) + component.registry.register('broken_component', BrokenComponent) def test_variable_outside_slot_tag_is_error(self): with self.assertRaises(TemplateSyntaxError): @@ -634,3 +640,25 @@ class TemplateSyntaxErrorTests(SimpleTestCase): {% slot "header" %}{% endslot %} """ ) + + def test_slot_with_no_parent_is_error(self): + with self.assertRaises(TemplateSyntaxError): + Template( + """ + {% load component_tags %} + {% slot "header" %}contents{% endslot %} + """ + ).render(Context({})) + + def test_isolated_slot_is_error(self): + with self.assertRaises(TemplateSyntaxError): + Template( + """ + {% load component_tags %} + {% component_block "broken_component" %} + {% slot "header" %}Custom header{% endslot %} + {% slot "main" %}Custom main{% endslot %} + {% slot "footer" %}Custom footer{% endslot %} + {% endcomponent_block %} + """ + ).render(Context({}))