-
Notifications
You must be signed in to change notification settings - Fork 15
Add option to eliminate unused classes from DOM #32
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
Conversation
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
?
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.
@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.
- 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.
- 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.
- The JSX should be concerned with the semantic meaning rather than the layout. This means that
idHeader
is better thanshortNumberWidth
for a<th>
element. - I might want to add more styles than just width to a header. This is another reason why
idHeader
is better thanshortNumberWidth
for a<th>
element, as I avoid having to refactor the JSX to add styling. - 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add couple tests:
- comment(s) inside block
- comment(s) inside selector before/after
- invalid composes (i.e. not reference)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Do you separate test cases or should everything go in these existing files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add them here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
dec5b56
to
91954d8
Compare
I will review this in near future
91954d8
to
9c23834
Compare
I'm new to this repository (never even having heard of CSS Modules a week ago), so please review this pull request carefully.
References
Concerns
Naming
I'm not 100% certain about the option name
exportEmptyLocals
. I first planned to call itomitEmptyLocals
so thatfalse
could be the default, but when reviewingcss-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 asundefined
. 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 inpostcss-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 topostcss-modules-scope
. I have written that code but would like to see what happens with this pull request before creating another one.