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

[Bug]: Minifying CSS deletes blocks of code #180

Open
chrisstaite-menlo opened this issue Aug 19, 2024 · 6 comments · May be fixed by #181
Open

[Bug]: Minifying CSS deletes blocks of code #180

chrisstaite-menlo opened this issue Aug 19, 2024 · 6 comments · May be fixed by #181

Comments

@chrisstaite-menlo
Copy link

What happened?

When running the minifier with the following input:

<style type="text/css">
{% ignore1 %}
a {
    {% ignore2 %}
}
{% ignore3 %}
</style>

And settings:

{
   ignoreCustomFragments: [
      /\{%[\s\S]*?%\}/
   ],
   collapseWhitespace: true,
   conservativeCollapse: true,
   minifyCSS: {
      level: 0
   },
}

The output is:

<style type="text/css"> {% ignore1 %} a{ {% ignore2 %} }</style>

Where it should be:

<style type="text/css"> {% ignore1 %} a{ {% ignore2 %} } {% ignore3 %} </style>

Full test program:

const { minify } = require('html-minifier-terser');

const html = `<style type="text/css">
{% ignore1 %}
a {
    {% ignore2 %}
}
{% ignore3 %}
</style>`;

const output = minify(html, {
  ignoreCustomFragments: [/\{%[\s\S]*?%\}/],
  collapseWhitespace: true,
  conservativeCollapse: true,
  minifyCSS: {
    level: 0,
  },
}).then(console.info);

Version

7.2.0

What browsers are you seeing the problem on?

No response

Link to reproduce

https://stackblitz.com/edit/node-hnsagk?file=index.js

Relevant log output

No response

Willing to submit a PR?

None

chrisstaite-menlo added a commit to chrisstaite-menlo/html-minifier-terser that referenced this issue Aug 19, 2024
If there are multiple ignored CSS blocks within a single clean-css
warning message, only the first is replaced with the ignore code.
This means that some ingored blocks are simply deleted from the output.

The problem is simply that the RegExp is only applied once, despite
having the global flag set on it.  This is fixed by calling `exec` on
the RegExp until it yields no further matches.

Added a test for this error case.

Fixes terser#180
@chrisstaite-menlo chrisstaite-menlo linked a pull request Aug 19, 2024 that will close this issue
chrisstaite-menlo added a commit to chrisstaite-menlo/html-minifier-terser that referenced this issue Aug 19, 2024
If there are multiple ignored CSS blocks within a single clean-css
warning message, only the first is replaced with the ignore code.
This means that some ingored blocks are simply deleted from the output.

The problem is simply that the RegExp is only applied once, despite
having the global flag set on it.  This is fixed by calling `exec` on
the RegExp until it yields no further matches.

Added a test for this error case.

Fixes terser#180
chrisstaite-menlo added a commit to chrisstaite-menlo/html-minifier-terser that referenced this issue Aug 21, 2024
If there are multiple ignored CSS blocks within a single clean-css
warning message, only the first is replaced with the ignore code.
This means that some ingored blocks are simply deleted from the output.

The problem is simply that the RegExp is only applied once, despite
having the global flag set on it.  This is fixed by calling `exec` on
the RegExp until it yields no further matches.

Added a test for this error case.

Fixes terser#180
chrisstaite-menlo added a commit to chrisstaite-menlo/html-minifier-terser that referenced this issue Aug 21, 2024
If there are multiple ignored CSS blocks within a single clean-css
warning message, only the first is replaced with the ignore code.
This means that some ingored blocks are simply deleted from the output.

The problem is simply that the RegExp is only applied once, despite
having the global flag set on it.  This is fixed by calling `exec` on
the RegExp until it yields no further matches.

Added a test for this error case.

Fixes terser#180
@Nantris
Copy link

Nantris commented Aug 22, 2024

Yes indeed, I'm seeing issues too since yarn upgrade which only affected our sub-dependencies. I'm guessing this might be a cssnano issue? We use Emotion for CSS in JS and I'm guessing they're not playing nice.

I'm not certain our issues are identical, but I suspect they come from the same source since disabling minifyCSS resolves it.

chrisstaite-menlo added a commit to chrisstaite-menlo/html-minifier-terser that referenced this issue Aug 27, 2024
If there are multiple ignored CSS blocks within a single clean-css
warning message, only the first is replaced with the ignore code.
This means that some ingored blocks are simply deleted from the output.

The problem is simply that the RegExp is only applied once, despite
having the global flag set on it.  This is fixed by calling `exec` on
the RegExp until it yields no further matches.

Added a test for this error case.

Fixes terser#180
@Nantris
Copy link

Nantris commented Nov 2, 2024

Regardless of the validity of their example, this is a real issue - or at least was when I commented.

@DanielRuf
Copy link
Contributor

DanielRuf commented Nov 2, 2024

Yes indeed, I'm seeing issues too since yarn upgrade which only affected our sub-dependencies.

Can you clarify? Because I've checked and the problem is not new.

@DanielRuf
Copy link
Contributor

I think that we should refactor the css logic, because there are more problems which arise when custom fragments are used:

<style type="text/css">
{% ignore1 %}
a {
    {% ignore2 %}
}
.a {
    {% ignore2 %}
}
{% ignore3 %}
</style>
<style type="text/css"> {% ignore1 %} a{} {% ignore2 %} .a{ {% ignore2 %} }</style>

And with the patch from the PR:

<style type="text/css"> {% ignore1 %} a{} {% ignore2 %} .a{ {% ignore2 %} } {% ignore3 %} </style>

@Nantris
Copy link

Nantris commented Nov 2, 2024

My original issue seems to be resolved now, so whatever broke it seems to have been fixed since then. It was definitely new around the time I posted though. I see the repro hasn't changed, so it could have been caused by yet another subdependency of this one if I had to guess.

@chrisstaite-menlo
Copy link
Author

chrisstaite-menlo commented Dec 3, 2024

@rogeriopvl, @sibiraj-s is it possible to progress the PR I have attached to this bug at all?

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 a pull request may close this issue.

3 participants