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

add customFragmentsInScripts option #716

Open
wants to merge 1 commit into
base: gh-pages
Choose a base branch
from

Conversation

gzzhanghao
Copy link

It leads to an issue with following html fragment:

<script>
function a() {
  b()
  <?php if (false): ?>
    c()
  <?php endif; ?>
}
</script>

After minified, it becomes:

<script>function a(){b(),}</script>

The trailing comma leads to a syntax error.

The only method I found is to prevent minifying javascripts when there's a customFragment inside the script tag.

@alexlamsl
Copy link
Collaborator

Can this not be implement with minifyJS by the use of function(text, inline){}?

@gzzhanghao
Copy link
Author

@alexlamsl I believe not :(

Custom fragments were replaced by the placeholders at that moment, so you can't easily find out whether there's a customFragment inside the script.

Setting sequence: false for minifyJS may help in this issue, but I think it's better to have such an option to prevent other potential issues.

@alexlamsl
Copy link
Collaborator

Right you are. 👍

I think rather than introducing this flag, if we can literally make this custom fragment case works by default that would be better. So if this option you've introduced work without any regressions, we might as well have this behaviour by default.

Failing that, I still prefer offloading this to a custom minifyJS processor if possible, if not because we can employ the same tricks for minifyCSS. After all, uglify-js and clean-css do not have the same sort of ignoreCustomFragments functionality as html-minifier does.

Would this work if I were to restore all the substitutions before passing the script string to function(text, inline) {...}?

@alexlamsl
Copy link
Collaborator

So coming back to this. One way to think of it is that uglify-js and clean-css should be fully responsible for the content, so we should just restore any custom fragment substitutions and pass the origin string directly at them. This however would break a lot of existing cases where the current logic does work properly, and many of which produce good savings.

So I think the flag you've introduced is probably quite a good solution to this issue. A few things:

  • the flag currently only takes care of JavaScript, but we do have similar issues with CSS as well
  • one option is to make two flags, one for JS and one for CSS
  • can we have the flag the other way round, i.e. set to true to pass custom fragments as-is?
  • please exclude dist/htmlminifier.js from the commit

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.

3 participants