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

Fix bug 1692423: Replace broken CSS compressor with a no-op compressor #1868

Merged
merged 4 commits into from Feb 12, 2021

Conversation

mathjazz
Copy link
Collaborator

@mathjazz mathjazz commented Feb 11, 2021

The + and - operators in CSS calc() must be surrounded by whitespace:
https://developer.mozilla.org/en-US/docs/Web/CSS/calc()#notes

The yuglify CSS compressor we use removes them and breaks CSS.
I tried yuicompressor instead, which has the same bug: yui/yuicompressor#59

Other CSS compressors I've checked look (even more) outdated. I haven't tested any.

The only other instance of calc() in our code uses - instead of +, which is fine.

The + and - operators in CSS calc() must be surrounded by whitespace:
https://developer.mozilla.org/en-US/docs/Web/CSS/calc()#notes

The yuglify CSS compressor we use removes them and breaks CSS.
I tried yuicompressor instead, which has the same bug:
yui/yuicompressor#59

Other CSS compressors I've checked look (even more) outdated.
I haven't tested any.

The only other instance of calc() in our code uses - instead of +,
which is fine.
@mathjazz mathjazz requested a review from julen February 11, 2021 20:56
@mathjazz mathjazz changed the title Avoid using calc(A + Npx) to circumvent bug in a CSS compressor Avoid using calc(A + Npx) to circumvent a bug in a CSS compressor Feb 11, 2021
@Pike
Copy link
Collaborator

Pike commented Feb 11, 2021

I mentioned this on matrix, I don't think we should constrain our CSS by bugs in that pipeline. In particular for CSS, where there's just not much to win, IMHO.

https://django-pipeline.readthedocs.io/en/latest/compressors.html#no-op-compressors suggests that we can just use a no-op css compressor to cat the files together.

If we're really concerned, maybe there's a trick to compress the cat'ed css files with a transfer encoding.

@mathjazz
Copy link
Collaborator Author

https://django-pipeline.readthedocs.io/en/latest/compressors.html#no-op-compressors suggests that we can just use a no-op css compressor to cat the files together.

Agreed. I've tested this on stage and it works as expected.

@mathjazz mathjazz requested a review from Pike February 11, 2021 23:29
@@ -476,6 +476,7 @@ def path(*args):
"STYLESHEETS": PIPELINE_CSS,
"JAVASCRIPT": PIPELINE_JS,
"JS_COMPRESSOR": "pipeline.compressors.terser.TerserCompressor",
"CSS_COMPRESSOR": "pipeline.compressors.terser.NoopCompressor",
Copy link
Collaborator

Choose a reason for hiding this comment

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

That one doesn't exist, it's in the parent, https://github.com/jazzband/django-pipeline/blob/master/pipeline/compressors/__init__.py#L256

Weird that that does something constructive on stage

Copy link
Collaborator

@julen julen left a comment

Choose a reason for hiding this comment

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

I fully agree with Axel. Changes look good to me 👍.

@mathjazz mathjazz changed the title Avoid using calc(A + Npx) to circumvent a bug in a CSS compressor Fix bug 1692423: Replace broken CSS compressor with a no-op compressor Feb 12, 2021
@mathjazz mathjazz merged commit b59d0e9 into mozilla:master Feb 12, 2021
@mathjazz mathjazz deleted the bug-fix-calc-var-compression branch February 12, 2021 09:15
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