Cache compiled templates within component by template_name #27
Conversation
django_components/component.py
Outdated
| def decorator(func): | ||
| return func | ||
| return decorator | ||
|
|
There was a problem hiding this comment.
I removed support for Python 2, so this can be removed!
There was a problem hiding this comment.
You should be able to get tests to pass by rebasing your code from latest master and force pushing to your branch.
django_components/component.py
Outdated
| @@ -55,15 +64,16 @@ def render_js_dependencies(self): | |||
| def slots_in_template(template): | |||
| return {node.name: node.nodelist for node in template.template.nodelist if is_slot_node(node)} | |||
|
|
|||
| def compile_instance_template(self, slots_for_instance): | |||
| @lru_cache(maxsize=128) | |||
There was a problem hiding this comment.
Is there a rationale for setting this to 128? Should be the number of unique times all your components get called with different values for template_name right?
Maybe we could make this configurable via the COMPONENTS setting that @danjac added?
There was a problem hiding this comment.
It's the default value, but I think it does make sense to make it configurable. I would bet that it won't matter for 95% of users, because they'll have < 10 templates ever used, but people who care at all will probably care a lot.
There was a problem hiding this comment.
Fully agree. So we add a default to COMPONENTS and always read from there?
tests/test_component.py
Outdated
| @@ -32,7 +32,7 @@ class Media: | |||
|
|
|||
| comp = SimpleComponent("simple_component") | |||
| context = Context(comp.context(variable="test")) | |||
| comp.compile_instance_template({}) | |||
| #comp.compile_instance_template({}) | |||
There was a problem hiding this comment.
This should be removed, not commented out, right?
|
I'm not sure that rebase worked. I see a merge commit and my own commits above. Anyways, and can select the commits I want in the list above and review them. About the code! This looks great. I think we need to add a default for TEMPLATE_CACHE_SIZE in app_settings the same way the two other properties are set there. Otherwise we're good to go. Thanks again! :) |
EmilStenstrom
force-pushed
the
master
branch
from
20dbf58
to
3919943
Compare
|
@rbeard0330 I messed up my git game, but your code is merged, I promise :) Thanks! Also released version 0.9 of the library with your fixes. |
As discussed in #26.
Only difference from what was discussed earlier is that I didn't invoke the compilation method when the template is compiled. It would require catching KeyErrors that are thrown when a client tries to look up something they expect to be in the context, and I don't think it will actually improve responsiveness--I'm not sure I have this right, but I think Django compiles a template immediately before the first time it renders it, so it's part of the first request-response cycle either way.
I used
lru_cachefor the caching, and defined a do-nothing decorator to avoid breaking Python 2.