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

Ignore custom fragments for minifyCSS #929

Closed

Conversation

felipemsantana
Copy link
Contributor

Closes #928

@felipemsantana
Copy link
Contributor Author

felipemsantana commented May 26, 2018

@alexlamsl as you can see here: 4eb22fe
Not all tests passed using this implementation, I think the only way is changing the clean-css tokenizer

Should specialComments: 'all' be forced in clean-css?
Or maybe show a warning if the value is not all.

@alexlamsl
Copy link
Collaborator

That will break as soon as there exists an instance of specialComments in the original code.

And if you try to use the numeral settings, the ordering of existing and injected specialComments would still break everything.

But as you stated in the code comment, upcoming [email protected] will have /* clean-css ignore */, so may be we should re-evaluate the fix for that.

Note that the same custom fragment issue would apply to JavaScript snippets since uglify-js won't handle invalid syntax either - though it would throw an error which causes html-minifier to fall back to verbatim output.

May be this PR should just be picking up any tokenizer warnings from clean-css and output the verbatim styles instead of the incorrect minified output?

@felipemsantana
Copy link
Contributor Author

I agree about how to handle of warnings of clean-css, will do that.

I think we should show a warning about specialComments if the value is not all.

@felipemsantana
Copy link
Contributor Author

These tests are failing because they depends of clean-css tokenizer removal of invalid chars and it emits warnings 😕

QUnit.test('remove comments from styles', function(assert) {

QUnit.test('remove CDATA sections from scripts/styles', function(assert) {

QUnit.test('decode entity characters', function(assert) {

@felipemsantana
Copy link
Contributor Author

felipemsantana commented May 27, 2018

https://travis-ci.org/kangax/html-minifier/jobs/384392625

To avoid breaking backwards compatibility I suggest to add a new option called ignoreWarnings that defaults to true, in next major version of html-minifier you could switch to false, I think this is the only place that has warnings but it won't be strictly bound to clean-css and can be used everywhere else.

@felipemsantana
Copy link
Contributor Author

@alexlamsl could you please review the changes and my suggestion? I can't use the minifyCSS option because of this bug.

@alexlamsl
Copy link
Collaborator

I couldn't see a way for this change to not break other existing use cases - may be [email protected] will give us a viable option of dealing with custom fragments.

Until then, I'll leave this open until a proper solution comes to mind.

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.

2 participants