Add option to eliminate unused classes from DOM

[sjoqvist]

I'm new to this repository (never even having heard of CSS Modules a week ago), so please review this pull request carefully.

References

• Eliminating unused classes css-modules#127
• Dropping empty classes css-modules#269

Concerns

Naming

I'm not 100% certain about the option name exportEmptyLocals. I first planned to call it omitEmptyLocals so that false could be the default, but when reviewing css-loader's set of related options (exportGlobals, exportLocalsConvention, exportOnlyLocals) the former one seemed more suitable. Furthermore, if this option will someday be the default, "omit" would be even more awkward. Still, this name doesn't tell the full story.

DOM-only fix

This fix only filters the exports, i.e. what ends up in the DOM. There are other tools available to strip CSS of empty declaration blocks, but please let me know if you think that such functionality nevertheless belongs here.

Side effects of empty imports

An imported selector with an empty declaration block (i.e. that doesn't contain any declarations nor a composes specifying another selector that does) renders as undefined. On the other hand, this is what happens already today when importing a non-existent selector. If the behavior is undesirable, it should probably be fixed in postcss-modules-extract-imports.

Other requirements

I believe that the correct approach would be to add an option to css-loader to make this configurable, which is then passed on to postcss-modules-scope. I have written that code but would like to see what happens with this pull request before creating another one.

[sjoqvist]

By the way, perhaps this pull request shouldn't close the issues, if it's going to be merged before the above-mentioned changes to css-loader?

[alexander-akait]

Hm, can you provide real use case for this? I am not against this option, I just want to understand the pros and cons of this.

[sjoqvist]

@alexander-akait My own use case is that I have a bunch of React components for tables with different content and layout. However, I still want them to have similar traits. Let's say that it looks similar to the following (untested).

import MyTableBody from './MyTableBody';
import styles from './MyTable.module.css';

export default function MyTable() {
  return (
    <table className={styles.normal}>
      <thead>
        <tr>
          <th className={styles.idHeader}>ID</th>
          <th className={styles.valueHeader}>Value</th>
          <th className={styles.descriptionHeader}>Description</th>
          <th className={styles.deleteHeader} />
        </tr>
      </thead>
      <MyTableBody />
    </table>
  );
}

/* _tables.module.css */
.table {
  table-layout: fixed;
}

.shortNumberWidth {
  width: 150px;
}

.singleButtonWidth {
  width: 65px;
}

.wideTextWidth {
  width: 800px;
}

/* MyTable.module.css */
.normal {
  composes: table from './_tables.module.css';
}

.idHeader {
  composes: shortNumberWidth from './_tables.module.css';
}

.valueHeader {
  composes: shortNumberWidth from './_tables.module.css';
}

.descriptionHeader {
  composes: wideTextWidth from './_tables.module.css';
}

.deleteHeader {
  composes: singleButtonWidth from './_tables.module.css';
}

What I want to accomplish is the following.

1. As I expect to end up with a lot of tables and I want to make use of the power of CSS Modules, I want to split the CSS into individual module files per component.
2. The width of, for example, a column that's meant to hold a short number should be defined in a single place. This means that I can't define it in the individual files but rather in an include file.
3. The JSX should be concerned with the semantic meaning rather than the layout. This means that idHeader is better than shortNumberWidth for a <th> element.
4. I might want to add more styles than just width to a header. This is another reason why idHeader is better than shortNumberWidth for a <th> element, as I avoid having to refactor the JSX to add styling.
5. Furthermore, the JSX shouldn't even have to know that there is an include file. It should be a concern of the CSS, and changes to the CSS hierarchies shouldn't cause changes to which files are included in JSX. Hence, the styles must be imported into MyTable.module.css.

In the example above, I end up with nine classes and each className includes two of them, despite only four of them being needed. In my opinion, the extra classes make the code both bloated and harder to understand. I also believe that when the internal structure of the code directly affects the output, the developer may be pressured into optimizing the code in counter-intuitive ways. Does this make sense?

[alexander-akait]

Let's add couple tests:

• comment(s) inside block
• comment(s) inside selector before/after
• invalid composes (i.e. not reference)

[sjoqvist]

Sounds good. Do you separate test cases or should everything go in these existing files?

[alexander-akait]

Let's add them here

[sjoqvist]

I've now force-pushed changes. I had to add the invalid composes in a separate directory, but the other tests were added to the original files. Not entirely sure if I got all your suggestions for tests correctly, but please have a look and let me know.

Some comments are removed, but that's what the current code does as well and I've made the tests reflect that. Since I added the true or "default" tests in a separate commit before the other changes, you can check out that commit and run yarn test to confirm it if you wish. You can also see that only the :export blocks differ:

diff test/test-cases/options-exportEmptyLocals-{true,false}/expected.css

Also, I should mention that I had overlooked the fact that comments can go inside the declaration blocks, so it was good that you pointed that out and I've updated the code accordingly. Let me know if there are any other special cases that I'm unaware of.

[alexander-akait]

I will review this in near future