Add compile-time option for class minification #246
Conversation
|
Hey @gilbox, Thanks for the PR! Mind signing our Contributor License Agreement? When you've done so, go ahead and comment Yours truly, |
gilbox
force-pushed
the
gil--minify-class-names
branch
from
0e12b8a to
84ce1cf
Compare
|
[clabot:check] |
|
CLA signature looks good 👍 |
README.md
Outdated
| @@ -594,6 +594,11 @@ css(styles2.globals); | |||
|
|
|||
| It isn't determinate whether divs will be red or blue. | |||
|
|
|||
| ## Minify class names | |||
|
|
|||
| Minify class names by setting the environment variable `process.env.APHRODITE_KEYS` | |||
There was a problem hiding this comment.
Is there a reason we want to add an additional env variable, instead of just using process.env.NODE_ENV === "production"?
There was a problem hiding this comment.
The reason I added another env variable is because it seems reasonable that someone might not want to minimize class names in production in order to make debugging production-only problems easier.
There was a problem hiding this comment.
I'm a little hesitant to require that folks use a new env variable in order to benefit from this.
There was a problem hiding this comment.
no problem, i'll change it to use NODE_ENV
src/exports.js
Outdated
| // TODO(emily): Make a 'production' mode which doesn't prepend | ||
| // the class name here, to make the generated CSS smaller. | ||
| _name: `${key}_${hashObject(val)}`, | ||
| _name: process.env.APHRODITE_KEYS !== 'MINIFIED' ? |
There was a problem hiding this comment.
Another thing that we were considering doing is making something like css(styles.a, styles.b) generate only a single hash class name, like _abcdef instead of combining two hashes like _abcdef-O_o-_012345. Not sure if you'd like to try tackling that here as well, but leaving a TODO about it would be nice if not. :)
If we do want to tackle it, we should figure out whether we need to make our hash longer. Currently, our hash is 6 base-36 characters, which means we'd expect collisions once there's ~47k styles on the page. Is that enough? On the largest KA pages, we end up seeing ~500 independent styles, but our website isn't very SPA-y, so the stylesheet gets cleared out pretty often. If we add uppercase letters, that bumps us up to ~240k styles on a page before seeing a collision.
There was a problem hiding this comment.
I was hoping to combine hashes in a separate PR. I'll add the TODO for now.
There was a problem hiding this comment.
Okay, great! Doing it separately sounds good :)
|
Thanks! Looking forward to your other PR :) |
|
One potential issue with using this optimization is that it dramatically increases the probability of hash collision. I believe when we were originally considering doing this, we planned on using a longer hash when omitting the class name. |
| _name: `${key}_${hashObject(val)}`, | ||
| // TODO(gil): Further minify the -O_o--combined hashes | ||
| _name: process.env.NODE_ENV === 'production' ? | ||
| `_${hashObject(val)}` : `${key}_${hashObject(val)}`, |
There was a problem hiding this comment.
Any reason to keep the leading _ here?
There was a problem hiding this comment.
class names can't start w/digits
|
@jlfwong In this PR, we're still doing the |
|
Is it possible to avoid minifying even if the variable |
|
@luixxiul when this was discussed we didn't have a specific use-case in mind. Maybe open a PR to add a new environment variable that turns off minification? |
|
That would be great. IMHO the info that the option is enabled by default should have been added to CHANGELOG.md. |
|
Yeah, unfortunately the CHANGELOG.md is a couple of versions out of date. |
|
I opened a new issue for that: #261 |
address this existing todo:
@lencioni