"context" is empty in template method

[danjac]

So I have this component:

class Svg(component.Component):
    def context(self, name: str, css_class: str = "", title: str = "", **attrs) -> Dict:
        return {"name": name, "css_class": css_class, "title": title, **attrs}

    def template(self, context: Dict) -> str:
        print(context)
        return f"svg/_{context['name']}.svg"

The template name is assigned dynamically depending on the name of the SVG:

{% component "svg" name="download" %}

However the template() method raises a KeyError, as the key "name" is not in the context: it's empty.

What is the order of execution here? Is template() called before context() ? If so, how do I access variables to be able to set the template name dynamically?

[danjac]

Looks like there's some optimization happening here in component.py:

    component_template = get_template(self.template({}))

So basically the "context" argument is pointless as it's always going to be {}, and is misleading as it gives the impression that the template context is available at rendering time.

If that's the intention, it kind of makes having "template" a method pointless as well: why not just make it a class attribute?

I'd probably want to make "template" an attribute instead, with a get_template() method that allows returning a template at render time.

[EmilStenstrom]

@rbeard0330 Is this related to your latest optimization?

[EmilStenstrom]

@danjac This is a bug. The component you've written should work as you intended it to. It's actually a good example that we should as as a simple acceptance test. I have a lot on my plate right now, but give me a couple of days and I'll have a look.

[rbeard0330]

@rbeard0330 Is this related to your latest optimization?

Yes, definitely. I think I mentioned in the PR that the change was breaking for this kind of use--fundamentally, you can't compile the component when the rest of the template is compiled if the identity of the component template changes each time the template is rendered, and you'll need to compile it fresh every render. A couple thoughts:

• The native {% include %} tag can cover a lot of potential use cases by passing it a dynamic template to load.
• The easiest fix would probably be to add a flag as either a setting or a class attribute of each Component that would control whether the component tries to compile itself at compile time (with an empty context) or waits until render time and uses the full context.
• Agreed that, as currently written, the get_template method doesn't really need to be a method, but removing it or changing its signature would break existing code.

[danjac]

Native "include" probably covers the use-case best here I think, and I wouldn't want to add complexity or performance regressions for this use case.

It would probably be a good idea to remove the "context" arg for template if it doesn't really do anything, or at least make it optional for backward compatibility.

[EmilStenstrom]

I wonder if the best way could be to look for a template method, and if that exists skip the pre-compilation? Shouldn't that give us the best of both worlds?

I do think that the code as danjac assumed should work, actually should.

[danjac]

@EmilStenstrom sure, but the documentation should be clear about the performance hit (if it's significant).

[EmilStenstrom]

If we should talk about a performance hit or not I think depends on what we compare to. If we're still faster than include, then a better way to describe it is "added optimization" if using a static template. One way to measure this could be to a benchmark comparing this to an included template with a dynamic template, just to see what we're comparing it to. This is also what #14 asks for. When @rbeard0330 added this (great) optimization, there was a 50% improvement in rendering speed in that particular benchmark.

[rbeard0330]

I wonder if the best way could be to look for a template method, and if that exists skip the pre-compilation? Shouldn't that give us the best of both worlds?

I do think that the code as danjac assumed should work, actually should.

This would require adding a different interface for specifying a static template (and I agree that a template_name variable would be the way to do it), and then people would need to rewrite their components to use it. We should bench it to be sure, but I think it will also perform worse than a dynamic include. The reason is that Django caches templates, so it should only have to compile each include once, and then it can reuse the compiled template next time it's needed.

Perhaps we could imitate Django by compiling template at render-time, and then cache compiled templates based on what the template method returns? If your template method is just returning a static string, then you still only compile it once, and even if it's dynamic template, we would still only render once per different template name.

[EmilStenstrom]

@rbeard0330 Caching sounds ideal now that you mention it. I guess we could even "prime" the cache with an empty context, to get speedup even at the first request for static templates, at the expense of some wasted caching in case the template actually is dynamic.

[rbeard0330]

I worked on this a bit this evening. It's pretty easy to do with lru_cache, but that's not supported in Python 2.

[danjac]

Why is there a need to support Python 2 in 2021, especially a new library?

[EmilStenstrom]

In 2015, when I started this projekt, Python 2 was still supported by Django. If it makes things tricky now, we can definitely drop it. Let's do it in a separate PR though.

[danjac]

Didn't realize the project was 5 years old :-)

Might be worth first dropping Python 2 support first so you can use lru_cache and other Python 3 improvements. Django itself no longer supports Python 2.

[EmilStenstrom]

I just dropped support for all Python and Django versions not officially supported my Django itself. This means we're now Python 3.6+ and Django 2.2+. Hope this makes things easier to hack on going forward :)

[EmilStenstrom]

@danjac I just released version 0.9 of the django-reusable-components package. Please try your code again and let us know how it works!

[EmilStenstrom]

I just added a test for the dynamic template case, to make sure we don't accidentally break this case again.