Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make compatible with ManifestStaticFilesStorage #157

Merged
merged 3 commits into from Sep 29, 2021

Conversation

jrief
Copy link
Owner

@jrief jrief commented Jul 27, 2021

No description provided.

Copy link

@mauritsvanrees mauritsvanrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the updated README from this PR fixed the incompatibility for me: setting SASS_PROCESSOR_STORAGE and SASS_PROCESSOR_STORAGE_OPTIONS to the given values. So that part of the PR is good, I would say.

The new code is not actually needed in my case it seems, but the changes make the working clearer, so it seems good. I don't really know this package though: another colleague introduced this as dependency in the project.

Quick background:

  • Old situation: Django 3.0 with django-sass-processor 0.8.2. My old settings worked fine.
  • New situation: Django 3.2 with django-sass-processor 1.0.1. My old settings gave errors like ValueError: Missing staticfiles manifest entry for 'images/favicon.png'. With the updated settings, it is fine again.

@Natureshadow
Copy link
Collaborator

Huh. I have not yet understood what the special case with the manifest storage is. Maybe we just need a second storage class the uses the ManifestStorageMixin?

I will read up on this in the following days.

N.B. It would be great if you could separate whitespace and typo changes that are not related into separate commits to make handling of potential bisects, reverts and the like in the future easier.

@jrief jrief merged commit 3a1293b into master Sep 29, 2021
@jrief
Copy link
Owner Author

jrief commented Sep 29, 2021

Since @mauritsvanrees and myself experienced an improvement using these code changes, I'm going to merge this now and release version 1.1.

@jrief jrief deleted the manifest-static-files-storage branch September 29, 2021 08:05
@jrief
Copy link
Owner Author

jrief commented Jan 10, 2023

@Natureshadow Without this PR, CSS files delivered by Django are unhashed. This may result in delivering outdated CSS files, since browsers and proxies are not informed to use a newer version.
Since you experienced problems in combination with S3, I changed this feature. This branch may solve both problems.

@mauritsvanrees could you please test with the new branch and report your experience.

@Natureshadow
Copy link
Collaborator

Natureshadow commented Jan 10, 2023 via email

@jrief
Copy link
Owner Author

jrief commented Jan 10, 2023

here are my relevant configurations in settings.py:

from pathlib import Path

BASE_DIR = Path(__file__).resolve().parent.parent

INSTALLED_APPS = [
    ...
    'sass_processor',
    ...
]

STATIC_ROOT = BASE_DIR / 'workdir/static'  # or whereever you keep temporarily compiled files
STATIC_URL = '/static/'
STATICFILES_FINDERS = [
    'django.contrib.staticfiles.finders.FileSystemFinder',
    'django.contrib.staticfiles.finders.AppDirectoriesFinder',
    'sass_processor.finders.CssFinder',
]

# if you want to access files in node_modules directly
STATICFILES_DIRS = [
    ('node_modules', BASE_DIR / 'node_modules'),
]
STATICFILES_STORAGE = 'django.contrib.staticfiles.storage.ManifestStaticFilesStorage'
# after applying this path, this will change to:
# STATICFILES_STORAGE = 'sass_processor.storage.ManifestStaticFilesStorage' 

 # optional, if you `@import ...` in SCSS from special paths, otherwise leave empty
SASS_PROCESSOR_INCLUDE_DIRS = ['path/to/static']

SASS_PROCESSOR_FAIL_SILENTLY = not DEBUG
SASS_PROCESSOR_STORAGE = 'django.contrib.staticfiles.storage.FileSystemStorage'
SASS_PROCESSOR_STORAGE_OPTIONS = {
    'location': STATIC_ROOT,
    'base_url': STATIC_URL,
}

@Natureshadow
Copy link
Collaborator

Natureshadow commented Jan 10, 2023

SASS_PROCESSOR_STORAGE = 'django.contrib.staticfiles.storage.FileSystemStorage'

That line forcefully disables the ManifestStaticFilesStorage. Leaving out this line makes compilescss use the configured STATICFILES_STORAGE and yields a style.0123456789abcd.css as expected.

The issue here is something else: ManifestStaticFilesStorage needs some manual work to generate the hashes. collectstatic does this by calling post_process on the storage after copying files.

So, the following two scenarios work flawlessly:

  • DEBUG = True and using the template tag → no hash generated, no hash used, file works without hash.
  • DEBUG = False and run compilecss and collectstatic → file generated at source location, hash generated on collectstatic, file works with hash.

All of this is clearly documented here.

So, I do not find the scenario that does not work as expected. What is the case that breaks for you?

@jrief
Copy link
Owner Author

jrief commented Jan 11, 2023

In version 1.2.2 (and Django-4.0.6), if I use

STATICFILES_STORAGE = 'django.contrib.staticfiles.storage.ManifestStaticFilesStorage'
SASS_PROCESSOR_STORAGE = STATICFILES_STORAGE  # commenting this setting has the same effect

STATIC_ROOT = 'path/to/staticfiles'

and I delete the folder path/to/staticfiles and then run ./manage.py compilescss with DEBUG = False, then
Django complains in django/django/contrib/staticfiles/storage.py:465 with a ValueError because it can't open the manifest JSON file.

btw., overriding the value manifest_strict = False doesn't help either.

@Natureshadow
Copy link
Collaborator

Natureshadow commented Jan 11, 2023 via email

@jrief
Copy link
Owner Author

jrief commented Jan 11, 2023

Did you also run collectstatic?

after running ./manage.py compilescss. Running it before doesn't make sense.

@Natureshadow
Copy link
Collaborator

Natureshadow commented Jan 11, 2023 via email

@jrief
Copy link
Owner Author

jrief commented Jan 11, 2023

And that still copies the file without adding it to the manifest?

No, ./manage.py compilescss aborts with a ValueError as mentioned before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants