EmilStenstrom / django-components

Create simple reusable template components in Django.
MIT License
953 stars 59 forks source link

python file names colide with global scope variables/modules #431

Closed EmilStenstrom closed 1 month ago

EmilStenstrom commented 1 month ago

Discussed in https://github.com/EmilStenstrom/django-components/discussions/430

Originally posted by **franciscobmacedo** April 11, 2024 # The problem If a component directory has python files in it that are named after some global scope variable name or module of another library, it overrides that variable. This causes a big problem: I can't name my python files after any global scope variable in my project. # Example I have a simple component called "select". the file structure is something like this: ``` sampleproject/ ├── components/ │ └── select/ │ ├── select.py # <- the problem │ └── template.html ├── sampleproject/ ├── manage.py └── requirements.txt ``` However, I also use celery, which uses the [select I/O module from the standard library](https://docs.python.org/3/library/select.html). When launching a celery task, it was causing this error: ```shell AttributeError: module 'select' has no attribute 'select' ``` Mostly because in my component `select.py` file I don't really have anything named `select`. For testing purposes I added an empty "django.py" file in that directory, and sure enough the same problem happens: ``` ImportError: cannot import name 'VERSION' from 'django' (/path/to/project/components/select/django.py) ``` **PS:** The variable names inside the python file and the registered component name don't cause any issues. It's only the python file(s) name(s) inside a component directory. # Current solution If I set the `COMPONENTS` settings variable to disable autodiscover, this is all fixed: ``` COMPONENTS = { "libraries": [ "components.select", ], "autodiscover": False, } ``` # Proposed ideas I understand the problem lies somewhere in the [import_file function](https://github.com/EmilStenstrom/django-components/blob/master/src/django_components/__init__.py#L39), where each component is registered as an independent module and therefore overriding already existing ones. I'm not adding this problem as an issue because there's a solution for it. However, I believe this is a limitation: I think one should be able to name a component python file "select.py" and not worry about repercussions. Or, at least, maybe this could be more explicit in the README.md? I don't want by all means complain, I love this project, so thank you very much for it!! Just thought this can be a problem for others as well. Thoughts?
EmilStenstrom commented 1 month ago

@franciscobmacedo I agree that this is not optimal behaviour. There's probably an easy fix that entails putting the components in the a component specific scope instead of the global one. I'll happily accept patches that changes that.

JuroOravec commented 1 month ago

Just a thought since I was dealing with imports yesterday (but for the template/media relative paths), and this seems like a similar issue.

If my project structure is

 sampleproject/
    ├── components/             
    │   └── select/           
    │       ├── select.py     # <- the problem
    │       └── template.html 
    ├── sampleproject/
    ├── manage.py
    └── requirements.txt

then I would expect that the Python import path to for select.py would be components.select.select. Hence IMO it should NOT collide with libraries like select.

I don't think I've used the autodiscover before. So just to be sure, @EmilStenstrom are the autodiscovered files imported and used elsewhere in the code? Or is the whole purpose just to load the component files, so that the component class is registered with @component.register("mycomponent")?

If it's the latter, then we could possibly skip this assignment since it has has no purpose for Django components:

sys.modules[spec.name] = module

Or alternatively we could try to resolve the autoimported files to a correct module name. E.g. assuming that I ran Django from within the sampleproject dir, then select.py would have relative path ./components/select/select.py, so we would turn that into components.select.select

I'll try to give this a try this weekend.

Btw @franciscobmacedo great work getting to the root of this issue!

franciscobmacedo commented 1 month ago

Thanks! It took me a while to find out what was the problem - I thought it was something with my python version 😂

EmilStenstrom commented 1 month ago

@JuroOravec The only purpose of autodiscover is not having to import the component classes manually (import them in your __init__.py files or whatever). So simply skipping putting them into the global scope sounds like a good idea.

franciscobmacedo commented 1 month ago

the modules lookup is used in some other places, specifically:

I believe the module path should be like you said @JuroOravec but I'm not sure how.

I made an initial PR, but it's still not working properly

JuroOravec commented 1 month ago

Hi, I made a separate MR for the fix, as I was just tinkering a lot on my fork: https://github.com/EmilStenstrom/django-components/pull/435

So, simply removing that line of code didn't work, because then the relative paths for templates feature didn't work (as @franciscobmacedo mentioned, in _resolve_component_relative_files). But what I found is that we can use importlib.import_module, and then everything works fine.

@franciscobmacedo in that case we can close https://github.com/EmilStenstrom/django-components/pull/434