Added skip_common_chunks argument to render_bundle to address duplicate script tags

[TeamTeaTime]

Added skip_common_chunks to render_bundle to address duplicate script tag issue #289. Uses context to store a list of all loaded tags by default and skips them if the option is set for that invocation of render_bundle.

I'm not sure if this should be included, but for me the use case is when I am composing a page out of templates that might include different entry points but with shared chunks. Ideally you just have one entry point but sometimes it might require refactoring that wouldn't be immediately necessary except for this issue.

Open questions I have:

1. What should the default value of skip_common_chunks be? False preserves existing behavior, but I'm not sure that's desirable.
2. Is it acceptable to add the overhead of checking and setting the context even for the single bundle use case? You need to track the loaded tags on every invocation of render_bundle, otherwise later calls with skip_common_chunks will not behave correctly. There could also be a config option set that globally enables/disables checking the context so that the overhead could be avoided.
3. Should the context variable name be set via a config option or otherwise be customizable?

[fjsj]

Please include a test for this new behavior.

[fjsj]

Thanks for the PR. Please check comments for improvements. About your questions:

• What should the default value of skip_common_chunks be?
  False for now to preserve existing behavior. We can change to True in the future with a major version update.

• Is it acceptable to add the overhead of checking and setting the context even for the single bundle use case?
  I think this overhead is very minimal if we use sets.

• Should the context variable name be set via a config option or otherwise be customizable?
  I think it's fine for it to be hardcoded. If we use a long name, it's very hard to clash with another library or user code.

[TeamTeaTime]

Just pushed another commit adding tests and addressing some other issues that came up.

The jinja Extension had to be modified because jinja doesn't know to treat the render_bundle decorated with takes_context=True differently. I added a context global in the jinja extension as a dict and explicitly pass it in so that the function signature for render_bundle doesn't change in jinja templates. This seems to work, but I don't use jinja so I may be unaware of some side effect of globals or their lifecycle could be different from my understanding.

Regarding the tests, I originally tried to write them in the style of the other tests, using request factory etc but it wasn't possible to access the modified context from my tests. I referenced the Flatpages Tests, which personally I thought was a little bit clearer since you have only the necessary tags and it's inline so you don't have to skip around as much. Let me know if you'd prefer to find a way to rewrite them in the other style and I can look into what's up with the context not getting set that way.

[karolyi]

I'm currently testing this with Jinja, skip_common_chunks=True seems to result in script tags not being output, investigating it and will come up with adjustments.

[karolyi]

So, after testing this out on Jinja, I have changed much of the code.

It is strongly advised against to modify context variables from within template tags, which is logical as it's got no place there, the context variables should be assigned at render time. What you can do though, is to take the original request variable (which is the same throughout all included templates), and put the set() on it.

I've fixed the code, for me this is what works with Jinja. Please test it with the built-in Django templates to see if it works right.

Here the output from git diff:

diff --git a/webpack_loader/contrib/jinja2ext.py b/webpack_loader/contrib/jinja2ext.py
index 217eceb..3fbc9d1 100644
--- a/webpack_loader/contrib/jinja2ext.py
+++ b/webpack_loader/contrib/jinja2ext.py
@@ -1,10 +1,16 @@
-import jinja2.ext
+from jinja2.ext import Extension
+from jinja2.runtime import Context
+from jinja2.utils import pass_context
 
 from ..templatetags.webpack_loader import render_bundle
 
 
-class WebpackExtension(jinja2.ext.Extension):
+@pass_context
+def _render_bundle(context: Context, *args, **kwargs):
+    return render_bundle(context, *args, **kwargs)
+
+
+class WebpackExtension(Extension):
     def __init__(self, environment):
         super(WebpackExtension, self).__init__(environment)
-        environment.globals["context"] = dict()
-        environment.globals["render_bundle"] = lambda *a, **k: jinja2.Markup(render_bundle(environment.globals["context"], *a, **k))
+        environment.globals["render_bundle"] = _render_bundle
diff --git a/webpack_loader/templatetags/webpack_loader.py b/webpack_loader/templatetags/webpack_loader.py
index 72c31d1..72aeb71 100644
--- a/webpack_loader/templatetags/webpack_loader.py
+++ b/webpack_loader/templatetags/webpack_loader.py
@@ -13,13 +13,12 @@ def render_bundle(context, bundle_name, extension=None, config='DEFAULT', suffix
         bundle_name, extension=extension, config=config,
         suffix=suffix, attrs=attrs, is_preload=is_preload
     )
-    if "webpack_loader_used_tags" not in context:
-        context["webpack_loader_used_tags"] = set()
-    used_tags = context["webpack_loader_used_tags"]
+    if not hasattr(context['request'], '_webpack_loader_used_tags'):
+        context['request']._webpack_loader_used_tags = set()
+    used_tags = context['request']._webpack_loader_used_tags
     if skip_common_chunks:
         tags = [tag for tag in tags if tag not in used_tags]
-    context["webpack_loader_used_tags"].update(tags)
-
+    used_tags.update(tags)
     return mark_safe('\n'.join(tags))
 
 @register.simple_tag

[karolyi]

An added requirement to access request in templates to have django.template.context_processors.request in the TEMPLATES['context_processors']['options'] of the settings, which is the default, as tested.

If we go this route, maybe a check on settings.TEMPLATES would be beneficial to avoid complaints from people who use that setting turned off. This applies for using the django template backend django.template.backends.django.DjangoTemplates.

In the case of using Jinja, I'm not sure what's the default setup, as I have no barebone Django+Jinja installation.

[TeamTeaTime]

I'm currently testing this with Jinja, skip_common_chunks=True seems to result in script tags not being output, investigating it and will come up with adjustments.

Thanks @karolyi . Let me know what you find. Looking at it now it seems that my approach for jinja won't work because I think in fact global variables are shared between requests.

Check out my latest commit and let me know if that does anything for you. I changed the jinja extension to use the jinja contextfunction decorator, which passes in a jinja Context object in. Since jinja Context is immutable, I have to check for the presence of the vars attribute, which seems to hold the actual mutable state. I don't love this, but this was the only way other than creating some kind of global data structure that tracks context between requests or something, and I'm not going to do that.

[karolyi]

@TeamTeaTime, please see my former comments.

[TeamTeaTime]

@TeamTeaTime, please see my former comments.

Sorry just saw them. If it isn't the jinja way to modify context variables then I'll defer to others who have more experience. I'm going to have less time to look at this today, but I can test it out later.

One note: The version of jinja that I have installed for this project is jinja 2.11.3. It doesn't have the pass_context decorator yet. I'm not sure what the correct version of jinja to be testing against is.

[fjsj]

Hi, thanks for all the work, I'll check this later, but maybe only on Monday. About jinja / django-jinja version, we can force the support of the highest version that also supports all Django LTS versions. @TeamTeaTime

[karolyi]

I'm fixing the tests with my variant and adding Jinja tests too.

FYI, I'm removing the py35-* envs:

py35-django22 installed: DEPRECATION: Python 3.5 reached the end of its life on September 13th, 2020. Please upgrade your Python as Python 3.5 is no longer maintained. pip 21.0 will drop support for Python 3.5 in January 2021. pip 21.0 will remove support for this functionality.

django-jinja dropped support for python3.5 too (as well as Jinja2<2):

https://github.com/niwinz/django-jinja/blob/d4d67ca5ef03d9cf181ad5eccc7c98e044681c5c/setup.py#L39

[fjsj]

Thanks a lot. Most commits are in #297, so I'm closing this one.