Try to re-use settings.py template loader config before using defaults

[ekaj2]

Fix #30

wrap_loaders checks if loaders[0][0] == "template_partials.loader.Loader". Which it doesn't if you had set TEMPLATES["OPTIONS"]["loaders"] to anything else. django-components suggests you do this in the installation section:

TEMPLATES = [
    {
        ...,
        'OPTIONS': {
            'context_processors': [
                ...
            ],
            'loaders':[(
                'django.template.loaders.cached.Loader', [
                    'django.template.loaders.filesystem.Loader',
                    'django.template.loaders.app_directories.Loader',
                    'django_components.template_loader.Loader',
                ]
            )],
        },
    },
]

So loaders[0][0] is actually "django.template.loaders.cached.Loader". Then wrap_loaders overrides it:

            already_configured = (
                loaders
                and isinstance(loaders, (list, tuple))
                and isinstance(loaders[0], tuple)
                and loaders[0][0] == "template_partials.loader.Loader"
            )
            if not already_configured:
                template_config.pop("APP_DIRS", None)
                default_loaders = [
                    "django.template.loaders.filesystem.Loader",
                    "django.template.loaders.app_directories.Loader",
                ]
                cached_loaders = [
                    ("django.template.loaders.cached.Loader", default_loaders)
                ]
                partial_loaders = [("template_partials.loader.Loader", cached_loaders)]
                template_config["OPTIONS"]["loaders"] = partial_loaders
            break

So the easy fix for anyone running into this is to wrap it yourself:

...
        "OPTIONS": {
            'loaders': [(
                'template_partials.loader.Loader', [(
                    'django.template.loaders.cached.Loader', [
                        'django.template.loaders.filesystem.Loader',
                        'django.template.loaders.app_directories.Loader',
                        'django_components.template_loader.Loader',
                    ])
                ]
            )],
...

This does that by default by wrap_loaders and uses the default config as a fallback.

[carltongibson]

We'd need a regression test here.

[ekaj2]

We'd need a regression test here.

For sure, just added one modeling after the other SimpleAppConfig test. Let me know if that's sufficient.

[garbelini]

It doesn't seem to me that the cached Loader is meant to be used as a child loader and passes calls directly through to it's children bypassing the caching mechanism. We noticed a considerable increase in our CI test time once we introduced it.

I might be missing something but this is what I understand currently:

1. The engine calls the top loader get_template method
2. the template_partials implements loader get_template and calls super().get_template https://github.com/carltongibson/django-template-partials/blob/71128aab3007be18e56f761509e6d536a5378c51/src/template_partials/loader.py#L37
3. Django's base.Loader will then proceed to iterate over all nested possible template_sources https://github.com/django/django/blob/4636baec179d8733e92c1eccfa669bd72d739735/django/template/loaders/base.py#L17-L24
4. Back in the template_partials Loader the get_template_sources forwards the calls down it's child loaders https://github.com/carltongibson/django-template-partials/blob/71128aab3007be18e56f761509e6d536a5378c51/src/template_partials/loader.py#L56-L58
5. At this point the first one is the cached.Loader that will, unfortunately, do exactly the same thing bypassing the actual method that caches templates get_templates https://github.com/django/django/blob/4636baec179d8733e92c1eccfa669bd72d739735/django/template/loaders/cached.py#L68-L70
6. This will eventually hit the filesystem.Loader that will return the template Origin https://github.com/django/django/blob/4636baec179d8733e92c1eccfa669bd72d739735/django/template/loaders/filesystem.py#L27-L45
7. Next call to the same template, the same will happen not touching the cached loader get_template

This is a simple test case showing the behavior.

class ChildCachedLoaderTest(TestCase):
    def test_child_cached_loader(self):
        wrap_loaders("django")
        engine = engines["django"].engine
        partial_loader = engine.template_loaders[0]
        self.assertEqual(type(partial_loader).__module__, "template_partials.loader")
        cached_loader = partial_loader.loaders[0]
        self.assertEqual(type(cached_loader).__module__, "django.template.loaders.cached")
        self.assertEqual(len(cached_loader.get_template_cache), 0)
        engine.get_template("example.html")
        self.assertEqual(len(cached_loader.get_template_cache), 1)  # <-- Failure

@override_settings(
    TEMPLATES=[{"BACKEND": "django.template.backends.django.DjangoTemplates", "APP_DIRS": True}]
)
class SanityCheckCachedTest(TestCase):
    def test_no_wrap_loaders(self):
        tpl_engine = django.template.engines["django"].engine
        # Cached tin the top level
        cached_loader = tpl_engine.template_loaders[0]
        self.assertIsInstance(cached_loader, django.template.loaders.cached.Loader)
        self.assertEqual(len(cached_loader.get_template_cache), 0)
        tpl_engine.get_template("example.html")
        self.assertEqual(len(cached_loader.get_template_cache), 1)

Not entirely sure how to work around or fix the issue. The Django test cases for the loaders don't seem to go beyond one level of nesting so it doesn't feel like it's something planned for. I just skimmed them so I might have overlooked something. It does feel quite unintuitive and a missed opportunity if that's the case.

[carltongibson]

Hi @garbelini — good spot. Yes, just eyeballing it, your analysis looks good.

Do you want to move your comment wholesale to a new issue (or perhaps a PR adding the two tests)?

The fix, I'd think, will be to call the cached loader's get_template (if len(self.loaders) == 1 and isinstance(...)) falling back to the current pathway otherwise.

[garbelini]

@carltongibson sounds good. I'll create an issue for it.

Thanks for the prompt reply!

[garbelini]

Created #36 I'll take a stab at the suggested fix and open a PR