cfware / babel-plugin-template-html-minifier

Minify HTML in tagged template strings using html-minifier
MIT License
63 stars 4 forks source link

partial lit-element CSS is not minified correctly #30

Closed LarsDenBakker closed 4 years ago

LarsDenBakker commented 5 years ago

In LitElement you can construct "partial" css which throws off the minifier.

For example given this input:

import {css} from 'lit-element';

const fontXL = css`
  20px;
`;

css`
  h1 {
    font-size: ${fontXL};
  }
`;

The output is this:

import{css}from'lit-element';const fontXL=css``;css`h1{font-size:${fontXL}}`;

For this input:

import {css} from 'lit-element';

const fontSize = css`
  20
`;

const fontXL = css`
  ${fontSize}px;
`;

css`
  h1 {
    font-size: ${fontXL};
  }
`;

the plugin exits with an error.

I create a MR with the failing test cases: https://github.com/cfware/babel-plugin-template-html-minifier/pull/29

I'd like to work on a solution, but I am unsure about the approach. There is a guarantee that eventually all css variables come together to form a valid stylesheet, so we could inline all the nested stylesheets and minify that. However that requires evaluating and parsing all dependencies of an individual module.

Do you have any ideas?

coreyfarrell commented 5 years ago

I'm not sure if/how this module would fix the issue. For things like this I generally use an ignored alias, for example:

import {css} from 'lit-element';

const partialCSS = css;
const fontSize = partialCSS`
  20
`;

const fontXL = partialCSS`
  ${fontSize}px;
`;

css`
  h1 {
    font-size: ${fontXL};
  }
`;

I've never done this with CSS before (so the above is not tested) but I've done this with html when doing <col> in separate templates (html-minifier was forcing it to be a child of a <colgroup> tag).

LarsDenBakker commented 5 years ago

That could work but unfortunately not all of the code is under my control. I'm in a big enterprise environment with a lot of teams writing code so I need a solution that just works.

coreyfarrell commented 5 years ago

Unfortunately the only other thing I can suggest as of now is to use the overrides option of babel config to prevent processing of CSS in sources which have partial CSS. Example:

{
  overrides: [
    {
      test: ['**/offending-source.js'],
      plugins: [
        ['template-html-minifier', {options_html_only}]
      ]
    },
    {
      exclude: ['**/offending-source.js'],
      plugins: [
        ['template-html-minifier', {options_everything}]
      ]
    }
  ]
}

I'm open to patches which would add support for partial CSS but I'm not sure how that could be safely accomplished. Maybe it could be reasonable to add a few new boolean options:

LarsDenBakker commented 5 years ago

After digging into this, I found out that clean-css which is used by html-minifier throws warnings when invalid CSS is being parsed. html-minifier ignores these warnings and returns the mangled CSS.

I was able to work around this by passing a custom minifyCSS function to html-minifier which manually calls clean-css and only returning the trimmed CSS on warning or error:

function customMinifyCSS(originalCSS) {
  const result = cleanCSS.minify(originalCSS);

  if (result.warnings.length > 0 || result.errors.length > 0) {
    return originalCSS.trim();
  }

  return result.styles;
}

I will do this for my own projects at least, do you think it should be an option in the babel plugin as well?

coreyfarrell commented 5 years ago

I'm concerned about the loss of wrapCSS / unwrapCSS when using this function. From what I can tell this will prevent minification of inline css <div style="overflow: auto">, the space before auto will not be removed. So if this babel plugin provided a way to provide a custom CSS function I think it would need to essentially copy the wrap/unwrap functionality:

Another potential concern I have is how clean-css might handle bindings inside CSS. Could this result in large stylesheets not being processed at all?

css`
* {
  /* does widthBinding produce a warning for the
     placeholder text as invalid for the width attribute? */
  width: ${widthBinding};
}
`
LarsDenBakker commented 5 years ago

You're right it would lose out on the wrap/unwrap, that's something that would need to be added back.

The snippet you posted will work fine because this plugin replaces bindings with a placeholder variable and clean-css will minify it without any warnings.

We have a design system which defines many css constants, css mixins, partial css fragments etc. to be reused. I notice that clean-css has issues parsing those. However on the consuming side constructions like this:

css`
${tableStyles}

.foo {
  ${fontXLMixin()}
  color: ${myBlueColor}
}
`

get minified just fine.

LarsDenBakker commented 5 years ago

I've been going with the custom minify function, and it's been working out pretty well so far.

Are you interested in having this in the plugin by default? It would involve copying over some code from html minifier.

coreyfarrell commented 5 years ago

I just checked, html-minifier is licensed MIT so I'm not strictly opposed to copying wrap/unwrap but could we first see if the maintainers of html-minifier are willing to provide those functions via module.exports? If we copied code I we'd have to list the copyright notice, I'm not sure the most appropriate way to do this. Maybe just put wrap/unwrap functions in a separate source file and add the html-minifier license in a header comment? In any case I think being able to import those functions would be better than copying.