EmilStenstrom / django-components

Create simple reusable template components in Django.
MIT License
1.1k stars 74 forks source link

Invalid block tag ... 'component'. Did you forget to register or load this tag? error #610

Closed danjac closed 2 days ago

danjac commented 2 weeks ago
  1. Added "django_components" to INSTALLED_APPS
  2. Set up TEMPLATES as follows:
TEMPLATES = [
    {
        "BACKEND": "django.template.backends.django.DjangoTemplates",
        "DIRS": [BASE_DIR / "templates"],
        "OPTIONS": {
            "builtins": [
                "django_components.templatetags.component_tags",
               # my builtins...
            ],
            "debug": env.bool("TEMPLATE_DEBUG", default=False),
            "context_processors": [
                "django.template.context_processors.debug",
                "django.template.context_processors.request",
                "django.contrib.auth.context_processors.auth",
                "django.template.context_processors.i18n",
                "django.template.context_processors.media",
                "django.template.context_processors.static",
                "django.template.context_processors.tz",
                "django.contrib.messages.context_processors.messages",
            ],
            "loaders": [
                (
                    "django.template.loaders.cached.Loader",
                    [
                        "django.template.loaders.filesystem.Loader",
                        "django.template.loaders.app_directories.Loader",
                        "django_components.template_loader.Loader",
                    ],
                )
            ],
        },
    }
]
  1. Created a simple component and added to template:
{% component "card" %}
....
{% endcomponent %}

Got the error "Invalid block tag on line 2: 'component'. Did you forget to register or load this tag?", indicating the template tag was not being loaded correctly

  1. Tried again with {% load component_tags %}, same error

Django 5.1, django-components==0.92

JuroOravec commented 2 weeks ago

Hey @danjac, my guess is that this error relates to the TagFormatter feature, where the component template tag is registered only if there is at least one component registered.

So my guess is that you didn't correctly register the card component.

Did you also configure STATICFILES_DIRS? That's what django_components uses to find the Python files with components. See step 5. in https://github.com/EmilStenstrom/django-components#installation

danjac commented 2 weeks ago

I'm trying if at all possible to avoid using STATICFILES_DIRS - I have no need to include CSS and JS with each component as I'm using Tailwind and Alpine and IMHO it's a "code smell" to include templates and Python files among static assets.

Is there a reason I can't just define them in the individual Python libraries e.g. under components ?

JuroOravec commented 2 weeks ago

Please go over the documentation. See the section on Autodiscovery. We use STATICFILES_DIRS to know which python files to search for.

It's possible not to use STATICFILES_DIRS, but then you have to import python files with the components yourself.

Like so in apps.py:

from django.apps import AppConfig

class MyAppConfig(AppConfig):
    name = "my_app"

    def ready(self) -> None:
        from components.card.card import Card
        from components.list.list import List
        from components.menu.menu import Menu
        from components.button.button import Button
        ...
VojtechPetru commented 2 weeks ago

... and IMHO it's a "code smell" to include templates and Python files among static assets.

Just to clarify, .py and .html files are not included in the static assets when you use django_components.safer_staticfiles (docs).

On a side note, I'm currently dealing with a somewhat related issue myself after updating from v0.28 (😱 a lot of progress you guys have made!), but I'll start a separate thread if I don't figure out a solution myself.

JuroOravec commented 2 weeks ago

@VojtechPetru Good point with django_components.safer_staticfiles, I didn't realize that's what @danjac had in mind.

Also, if, during the migration, there will be some non-obvious steps you will need to make, would you make a discussion and share them there? It could be helpful for others!

danjac commented 2 weeks ago

... and IMHO it's a "code smell" to include templates and Python files among static assets.

Just to clarify, .py and .html files are not included in the static assets when you use django_components.safer_staticfiles (docs).

On a side note, I'm currently dealing with a somewhat related issue myself after updating from v0.28 (😱 a lot of progress you guys have made!), but I'll start a separate thread if I don't figure out a solution myself.

I'm aware of the safer_staticfiles app, but it's still a code smell to have Python/Django templates loaded by default as static assets (i.e. insecure by default), then require a workaround to remove that insecurity.

JuroOravec commented 2 weeks ago

Hm, I agree it would be nice to get rid of safer_staticfiles before v1...

Had a look and what would be required is probably to write a custom Finder for STATICFILES_FINDERS, and then use that in STATICFILES_FINDERS:

STATICFILES_FINDERS = [
    # Default finders
    "django.contrib.staticfiles.finders.FileSystemFinder",
    "django.contrib.staticfiles.finders.AppDirectoriesFinder",
    # Django components
    "django_components.finders.ComponentAssetsFinder",
]

This means that we'd also need to define an additional setting for specifying where to look for components:

COMPONENTS = {
    "dirs": [
        BASE_DIR / "templates",
    ],
}

At this point, we could get rid of safer_staticfiles.

How it would work is that STATICFILES_DIRS would be used solely by django.contrib.staticfiles.finders.FileSystemFinder, and it would keep it's original behavior, which is, that it would load all the files within the given directories as assets.

On the other hand, django_components.finders.ComponentAssetsFinder would use the locations given in COMPONENTS.dirs and look up all files within the directories, BUT EXCLUDE python (and .pyc) files.

I have a lot on my plate already, so feel free to look into this. All interaction with STATICFILES_DIRS is in src/django_components/template_loader.py.

JuroOravec commented 2 weeks ago

Ok, I actually got some time, and got things working as described above :) There's couple of other tickets open, but I reckon we could get this merged in a week or two.

EmilStenstrom commented 2 weeks ago

Agree with @danjac's point that not being secure by default is a code smell. One obvious solution would be to move components out of static dirs altogether, and then somehow link into the collectstatic flow some other way...

@JuroOravec About the solution you have above with specifying a component dir: I would expect this to work like with template dirs, that we would automatically look for "components" directories in all app dirs as well.

JuroOravec commented 2 weeks ago

@EmilStenstrom Yes, same logic and same input, users would just use COMPONENTS.dirs instead of STATICFILES_DIRS for their components dirs.

One correction tho, we no longer automatically look into components dirs, this was removed in 0.85.

danjac commented 2 weeks ago

Was there a reason to remove the per-app autodiscover? As a Django developer this is the pattern I am used to (e.g. with Django admin discovery or app templates), along with a list of libraries in settings under COMPONENTS.libraries for additional components (e.g. a "card" component that might be used across apps). Aside from the security issue, keeping Python code under static files is very counter-intuitive.

Re JS/CSS lookup - we have the example in Django of Form.media which just assumes you keep the assets wherever else you keep your static files: https://docs.djangoproject.com/en/5.0/topics/forms/media/. Again, this would be the implicit assumption any Django developer would have. IMHO it's better to follow common framework conventions as much as possible, both to reduce complexity but also friction for people adopting the library.

JuroOravec commented 2 weeks ago

@danjac re per-app-components, it wasn't documented, and it wasn't entirely clear how it should work, so I removed it for simplicity. To be specific, it didn't behave like top-level componentsdir, where we automatically search the contents. We just imported [app]. components. So it still required users to import the nested components into [app]. components.py

So IMO if we want to bring it back, I suggest it should work the same way as top-level components, that we search the directory contents automatically.

As for Form.media, that's what's used internally when you specify JS / CSS files under Component.Media.js/css.

You totally can keep JS/CSS with other static files and then just point component's JS/CSS to those. There's only a convenience sprinkled on top, which allows you to specify the JS/CSS location relative to the python file.

danjac commented 2 weeks ago

You totally can keep JS/CSS with other static files and then just point component's JS/CSS to those. There's only a convenience sprinkled on top, which allows you to specify the JS/CSS location relative to the python file.

Sure, but if you can live without that convenience, you can do away with needing to use STATICFILES for anything.

@danjac re per-app-components, it wasn't documented, and it wasn't entirely clear how it should work, so I removed it for simplicity. To be specific, it didn't behave like top-level componentsdir, where we automatically search the contents. We just imported [app]. components. So it still required users to import the nested components into [app]. components.py

I would argue that autodiscover is a common Django idiom e.g. with top-level and per-app templates, template tags, Django admin, etc. Therefore having per-app discovery makes sense, with a way to specify module/package paths in settings (much like how template tag lookups work). Yes, it has to be documented, but it lets you lean on Django's existing mechanisms for autodiscovery and package lookup and would not be surprising to new users.

JuroOravec commented 2 weeks ago

Sure, but if you can live without that convenience, you can do away with needing to use STATICFILES for anything.

@danjac I'd be interesting to hear / see what's your setup, and how you manage complexity. As for my experience, the project I work on it's probably mid-sized, it's currently around 70 components, 50 endpoints / views, and 20 pages. I use VSCode (haven't tried Pycharm or similar). Previously I kept components and views in totally different apps, but I just had to scroll so much, it was easy for me to lose focus. Now I keep the app-specific components within the relevant app, split components to components and pages, and keep the component's JS / CSS / HTML next to it.

For me personally it definitely lowers the mental overhead, and I'd rather have one more configuration step up front than cook my brains.

I would argue that autodiscover is a common Django idiom e.g. with top-level and per-app templates, template tags, Django admin, etc. Therefore having per-app discovery makes sense, with a way to specify module/package paths in settings (much like how template tag lookups work). Yes, it has to be documented, but it lets you lean on Django's existing mechanisms for autodiscovery and package lookup and would not be surprising to new users.

I'm making a note of that!

danjac commented 2 weeks ago

Re STATICFILES: given your use case (keep JS and CSS in the same directory/package) I would suggest a) making "find in static files" an optional (non-default) add-in e.g. COMPONENTS.find_in_staticfiles=True and if it is enabled, raise an ImproperlyConfigured if safe_staticfiles is not in INSTALLED_APPS (or use a Django check to raise a warning/error).

In other words, make it an option, and enforce security checks if it is enabled.

JuroOravec commented 2 weeks ago

@danjac Hm, just to check I understand, are you talking about the case where python files are in STATICFILES_DIRS? So if we implemented this, where we'd configured component dirs via COMPONENTS.dirs and removed safe_staticfiles, then we wouldn't have to add COMPONENTS.find_in_staticfiles. Is that correct?

danjac commented 2 weeks ago

@danjac Hm, just to check I understand, are you talking about the case where python files are in STATICFILES_DIRS? So if we implemented this, where we'd configured component dirs via COMPONENTS.dirs and removed safe_staticfiles, then we wouldn't have to add COMPONENTS.find_in_staticfiles. Is that correct?

Yes, that's probably the better plan.

EmilStenstrom commented 2 weeks ago

@JuroOravec Thanks for correcting me regarding that we removed that strange, non-documented, behavior. I think the setup you describe is very sane: Per app-component dirs AND a global component dir for the global ones. I've seen many Django projects that has that setup for templates (app-specific + global). I wonder if we should make that the default for django_components as well? So if people put things in a components dir, things just work without any config.

I think what I'm suggesting is making this the default:

STATICFILES_FINDERS = [
    # Default finders
    "django.contrib.staticfiles.finders.FileSystemFinder",
    "django.contrib.staticfiles.finders.AppDirectoriesFinder",
    # Django components
    "django_components.finders.ComponentAssetsFinder",
]

COMPONENTS = {
        "find_components_in_apps": True,
    "dirs": [
        BASE_DIR / "components",
    ],
}
JuroOravec commented 1 day ago

This has now been released in v0.100 🎉

danjac commented 1 day ago

Great work, thank you!!!