EmilStenstrom / django-components

Create simple reusable template components in Django.
MIT License
1.16k stars 75 forks source link

django_components.component_registry.AlreadyRegistered error when using component as view. #368

Closed Fingel closed 9 months ago

Fingel commented 9 months ago

Hello! I found this library because I had the exact same use case in mind as outlined in this issue: https://github.com/EmilStenstrom/django-components/issues/349 I would like to use Django + HTMX, and being able to return a single component as a view fits perfectly. I believe this has the potential to become a very powerful paradigm.

It's mentioned in https://github.com/EmilStenstrom/django-components/issues/349#issuecomment-1891103065 that the author of the PR needed to explicitly register each component to avoid the AlreadyRegistered error. The pull request here by @dylanjcastillo https://github.com/EmilStenstrom/django-components/pull/366 mentions that a modification had to be made to not raise an error if the same component is registered twice.

I still get the error, so I'm wondering if this is not working as intended, or perhaps a different step needs to be taken? I haven't taken a look at the PR yet, but I'll be happy to debug.

Fingel commented 9 months ago

OK, I think I've found the root cause. The class hash function here: https://github.com/EmilStenstrom/django-components/blob/91b4accfebf74f3f7a8cf2ecab03789a1979d578/django_components/component.py#L93 will return 2 different hashes if a class is imported using a different path.

Take for example, a Todo component with a .py file under components/todo/todo.py. When using the regular @component.register("todo") wrapper, the return value of cls.__module__ is todo.

However, if you import the module in another file, such as urls.py like this:

from components.todo.todo import TodoComponent

urlpatterns = [...]

the return value of cls.__module__ will be components.todo.todo.

This is an issue because the documentation at https://github.com/EmilStenstrom/django-components/blob/91b4accfebf74f3f7a8cf2ecab03789a1979d578/README.md?plain=1#L449 instructs you to place your components urls.py in the top level components folder, and import the example competent like so:

from calendar import Calendar

However this import will not work if the urls.py module is in a directory above the calendar folder. You need to write it like this:

from components.calendar.calendar import Calendar

or use a relative import. Either way, the hash function will not match because the module will not simply be calendar.

The only way to get this to work is if where you are importing the module (such as in a urls.py) is in the same module as where the component is defined. And indeed, in the sample/test project that the component views tests use: https://github.com/EmilStenstrom/django-components/blob/91b4accfebf74f3f7a8cf2ecab03789a1979d578/sampleproject/components/urls.py

the urls.py file is in the same folder as greeting.py where the component is actually defined. So in this case, the hash function will match - but this does not match what the documentation says to do.

So I think there are two options:

  1. Update the documentation to say that the urls.py file needs to be in the same directory as the module that defines the component, or
  2. Make the hash function a bit more robust so that it stands up to imports from different paths.

EDIT:

I did some looking into this and I believe the root of this stems from the auto-detect and import that happens on init here: https://github.com/EmilStenstrom/django-components/blob/91b4accfebf74f3f7a8cf2ecab03789a1979d578/django_components/__init__.py#L36

No matter what I try, I cannot get the identity of the modules imported via autodetect to match the identity of classes imported later, such as in a urls.py. The full module path does not match. This makes it hard to make the class_hash method work.

As an example of this, I can make my project work by getting the component directly from the registry, thus circumventing the second attempt to register:

from django_components.component_registry import registry
urlpatterns = [path("components/todo", registry.get("todo").as_view(), name="todo")]

But this seems a little hacky.

Another option would be to make the class_hash method a little less resrictive:

    def class_hash(cls):
        return hash(cls.__name__)

This will still prevent someone from the (probably most common case) of trying to register a second component under the same component name, unless the class name they use also matches, but at some point you gotta just stop it 🤷‍♂️

EmilStenstrom commented 9 months ago

Hi @Fingel! Before going further into troubleshooting this: Are you sure you're using the latest version of the code? I JUST released 0.34 to pypi. Which version are you using? Does things work when you run the latest release?

Fingel commented 9 months ago

Yes, this is on Master. When I get home I’ll try and get you a PR with a failing test case. Probably what I should have done first before blasting you with a wall of text.

EmilStenstrom commented 9 months ago

@dylanjcastillo Is on it.

It was me who suggested this (faulty) change in the PR review, going with Dylan's original suggestions would have avoided this! :)

dylanjcastillo commented 9 months ago

@Fingel, I got the PR for this now. Please give it a try, and let us know if it works!

@EmilStenstrom, no worries. I should've included a specific test for this, and we have that now!

Fingel commented 9 months ago

Yes! It works now. Nice work. The python import system is so funky.