-
Notifications
You must be signed in to change notification settings - Fork 188
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
Minify combined class names #248
Minify combined class names #248
Conversation
src/exports.js
Outdated
@@ -21,7 +21,6 @@ const StyleSheet = { | |||
create(sheetDefinition /* : SheetDefinition */) { | |||
return mapObj(sheetDefinition, ([key, val]) => { | |||
return [key, { | |||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is going to be joined and hashed again, we can probably remove the leading underscore here now, right?
ping @lencioni addressed your comment and added other improvements |
ping @xymostech |
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.
Like @jlfwong mentioned in this comment: #246 (comment) we maybe want to be generating longer hashes now that we're combining class names, since we're increasing the risk of collisions. Unfortunately, it looks like the library we're using for hashing doesn't support generating longer hashes... Hmm. Does anyone have insight into how we might do that in a good way?
src/util.js
Outdated
@@ -132,7 +132,7 @@ export const stringifyAndImportantifyValue = ( | |||
// ordering of objects. Ben Alpert says that Facebook depends on this, so we | |||
// can probably depend on this too. | |||
export const hashObject = (object /* : ObjectMap */) /* : string */ => stringHash(JSON.stringify(object)).toString(36); | |||
|
|||
export const hashString = (string /* : string */) /* string */ => stringHash(string).toString(36); |
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.
Could you maybe add a quick docstring here, and maybe use this function inside of hashObject
so it's clear that they're supposed to be using the same hash algorithm?
If you need more than 32 bits of hash, you're kinda stuck, since js doesn't provide 64-bit integer types. You could go back to using a google hash: https://github.com/lovell/farmhash e.g. It has fingerprint64() which gives you a 64-bit hash (as a string), and is platform independent. I don't know how fast it is, though. Your only other solution would be to generate two separate 32-bit hashes and concatenate them. The most straightforward way to do this, if you're always hashing the same kind of object, is to add a 'salt' field to the object, and hash it once with salt=1 and a second time with salt=2 (or whatever). Of course, this will double your hashing time. farmhash is probably faster. :-) |
(Blah, maybe not possible -- it looks like farmhash is node-specific, and just a wrapper around some C code.) |
src/inject.js
Outdated
if (process.env.NODE_ENV === 'production') { | ||
className = processedStyleDefinitions.classNameBits.length === 1 ? | ||
`_${processedStyleDefinitions.classNameBits[0]}` : | ||
`_${hashString(processedStyleDefinitions.classNameBits.join())}`; |
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.
What if we append processedStyleDefinitions.classNameBits.length
? That would help reduce the likelihood of collisions.
…ngthHash to add extra hash bit in prod
ping @xymostech @lencioni |
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.
This seems good to me, but it looks like CI is red.
@xymostech what do you think?
@@ -243,6 +243,12 @@ const processStyleDefinitions = ( | |||
} | |||
}; | |||
|
|||
// Sum up the lengths of the stringified style definitions (which was saved as _len property) | |||
// and use modulus to return a single byte hash value. |
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.
This comment would be 💯 if it explained why we are adding the length to the hash.
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 className; | ||
if (process.env.NODE_ENV === 'production') { | ||
className = processedStyleDefinitions.classNameBits.length === 1 ? | ||
`_${processedStyleDefinitions.classNameBits[0]}` : |
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.
should we maybe be adding the length of the style definitions when there's only one class name, too? If only to make the hashes the same size/consistent?
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'm not sure what the advantage of that would be. Also, before this PR the hashes were different lengths--for example, from our homepage:
image_ay4wjb-o_O-background_1h6n1zu-o_O-fadeIn_3jddj2-o_O-backgroundSize_cover_176vplr
--so I imagine they still won't be the same size.
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.
oh, the other thing is that we'd be hashing a hash unnecessarily. is the additional expense justified?
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.
We wouldn't be hashing anything new, just adding the extra length char on, right? You're right though, this probably isn't important. :)
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.
oh, you're right double-hashing shouldn't be a thing
I think the travis failing was a fluke, I restarted it |
Okay, I'm gonna merge this! Sorry for the delay! Maybe once this lands, @lencioni or I could try updating aphrodite on our sites and see if anything breaks and how much smaller the styles get. |
That sounds good. I actually work with @gilbox so I'll let him do the honors on our site and report back here with anything interesting. 💃 |
Oh ha, I didn't realize! Sounds wonderful :) |
@xymostech could we get a release please 🙏 |
@gilbox Did you test things out on your site? I can make a release but I'd love for someone to do some real testing to make sure this doesn't horribly break things... |
ah ok I didn't realize that's what you meant. I'll give it a whirl |
Fixes regression introduced by Khan#248
I tested with #263, should be good to publish a new version |
ping @xymostech for the new version |
Hashes the combined class name to create a minified combined class name.
@lencioni
@xymostech
@jlfwong