protibimbok / django-vite-plugin

This plugin configures Vite for use with Django backend.
103 stars 13 forks source link

Mutable default argument #56

Closed maziar-dandc closed 3 months ago

maziar-dandc commented 4 months ago

https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments

https://github.com/protibimbok/django-vite-plugin/blob/691822109688d3e756468d7d5f092787875175b1/django/src/django_vite_plugin/utils.py#L91

Causing css styles to not be imported on the page.

protibimbok commented 4 months ago

This is supposed to prevent CSS files from being loaded more than once. Please provide me the way of replicating your problem.

maziar-dandc commented 4 months ago

I wasn't able to reproduce it reliably, but this is essentially the output:

4 out of 5 times: image

1 out of 5 times: (no css import) image

The reason it's unreliable might be because we run multiple django backend servers behind a load balancer, but essentially what happens is due to the mutable default argument, it thinks it's already imported the css file, and doesn't import it again.

mutable default argument essentially provides a python global variable, but what you want here is a per-page-render variable that ensures same css isn't imported on the same page twice.


I have pushed this fix on our staging/production, if it resolves the issue for good, I'll report back.

maziar-dandc commented 4 months ago

Yeah this has fixed the problem with phantom missing css problem.


New problem:

https://github.com/protibimbok/django-vite-plugin/blob/691822109688d3e756468d7d5f092787875175b1/django/src/django_vite_plugin/utils.py#L98

html += _get_css_files(VITE_MANIFEST[import_path], attrs, already_processed)

Had to change this as well to get the imports related css files recursively correctly.

Sample manifest.json using vite 5+: image

maziar-dandc commented 4 months ago

I can make these changes a PR if you want, lemme know.

protibimbok commented 3 months ago

Did you mean #57?

protibimbok commented 3 months ago

Please check if #58 solves the first problem. This was a weird python behavior I forgot.

maziar-dandc commented 3 months ago

Yeah these PRs solve both of the problems, thanks ❤️👍🏻