Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/exports.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* @flow */
import {mapObj, hashObject} from './util';
import {mapObj, hashString} from './util';
import {
injectAndGetClassName,
reset, startBuffering, flushToString,
Expand All @@ -20,10 +20,11 @@ export type MaybeSheetDefinition = SheetDefinition | false | null | void
const StyleSheet = {
create(sheetDefinition /* : SheetDefinition */) {
return mapObj(sheetDefinition, ([key, val]) => {
const stringVal = JSON.stringify(val);
return [key, {
// TODO(gil): Further minify the -O_o--combined hashes
_len: stringVal.length,
_name: process.env.NODE_ENV === 'production' ?
`_${hashObject(val)}` : `${key}_${hashObject(val)}`,
hashString(stringVal) : `${key}_${hashString(stringVal)}`,
_definition: val
}];
});
Expand Down
20 changes: 18 additions & 2 deletions src/inject.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import asap from 'asap';

import OrderedElements from './ordered-elements';
import {generateCSS} from './generate';
import {hashObject} from './util';
import {hashObject, hashString} from './util';

/* ::
import type { SheetDefinition, SheetDefinitions } from './index.js';
Expand Down Expand Up @@ -243,6 +243,13 @@ 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.
Copy link
Copy Markdown
Collaborator

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// We append this extra byte to the 32bit hash to decrease the chance of hash collisions.
const getStyleDefinitionsLengthHash = (styleDefinitions /* : any[] */) /* : string */ => (
styleDefinitions.reduce((length, styleDefinition) => length + styleDefinition._len, 0) % 36
).toString(36);

/**
* Inject styles associated with the passed style definition objects, and return
* an associated CSS class name.
Expand All @@ -269,7 +276,16 @@ export const injectAndGetClassName = (
if (processedStyleDefinitions.classNameBits.length === 0) {
return "";
}
const className = processedStyleDefinitions.classNameBits.join("-o_O-");

let className;
if (process.env.NODE_ENV === 'production') {
className = processedStyleDefinitions.classNameBits.length === 1 ?
`_${processedStyleDefinitions.classNameBits[0]}` :
Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor Author

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?

Copy link
Copy Markdown
Contributor

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. :)

Copy link
Copy Markdown
Contributor Author

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

`_${hashString(processedStyleDefinitions.classNameBits.join())}${
getStyleDefinitionsLengthHash(styleDefinitions)}`;
} else {
className = processedStyleDefinitions.classNameBits.join("-o_O-");
}

injectStyleOnce(
className,
Expand Down
6 changes: 4 additions & 2 deletions src/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,9 @@ export const stringifyAndImportantifyValue = (
prop /* : any */
) /* : string */ => importantify(stringifyValue(key, prop));

// Turn a string into a hash string of base-36 values (using letters and numbers)
export const hashString = (string /* : string */) /* string */ => stringHash(string).toString(36);

// Hash a javascript object using JSON.stringify. This is very fast, about 3
// microseconds on my computer for a sample object:
// http://jsperf.com/test-hashfnv32a-hash/5
Expand All @@ -131,8 +134,7 @@ export const stringifyAndImportantifyValue = (
// this to produce consistent hashes browsers need to have a consistent
// 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 hashObject = (object /* : ObjectMap */) /* : string */ => hashString(JSON.stringify(object));

// Given a single style value string like the "b" from "a: b;", adds !important
// to generate "b !important".
Expand Down
2 changes: 1 addition & 1 deletion tests/index_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ describe('StyleSheet.create', () => {
},
});

assert.equal(sheet.test._name, '_j5rvvh');
assert.equal(sheet.test._name, 'j5rvvh');
});
})
});
Expand Down
50 changes: 49 additions & 1 deletion tests/inject_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ import jsdom from 'jsdom';

import { StyleSheet, css } from '../src/index.js';
import {
injectAndGetClassName,
injectStyleOnce,
reset, startBuffering, flushToString, flushToStyleTag,
addRenderedClassNames, getRenderedClassNames
addRenderedClassNames, getRenderedClassNames,
} from '../src/inject.js';
import { defaultSelectorHandlers } from '../src/generate';

const sheet = StyleSheet.create({
red: {
Expand All @@ -34,6 +36,52 @@ describe('injection', () => {
global.document = undefined;
});

describe('injectAndGetClassName', () => {
it('uses hashed class name', () => {
const className = injectAndGetClassName(false, [sheet.red], defaultSelectorHandlers);
assert.equal(className, 'red_137u7ef');
});

it('combines class names', () => {
const className = injectAndGetClassName(false, [sheet.red, sheet.blue, sheet.green], defaultSelectorHandlers);
assert.equal(className, 'red_137u7ef-o_O-blue_1tsdo2i-o_O-green_1jzdmtb');
});

describe('process.env.NODE_ENV === \'production\'', () => {
let prodSheet;
beforeEach(() => {
process.env.NODE_ENV = 'production';
prodSheet = StyleSheet.create({
red: {
color: 'red',
},

blue: {
color: 'blue',
},

green: {
color: 'green',
},
});
});

afterEach(() => {
delete process.env.NODE_ENV;
});

it('uses hashed class name (does not re-hash)', () => {
const className = injectAndGetClassName(false, [prodSheet.red], defaultSelectorHandlers);
assert.equal(className, `_${prodSheet.red._name}`);
});

it('creates minified combined class name', () => {
const className = injectAndGetClassName(false, [prodSheet.red, prodSheet.blue, prodSheet.green], defaultSelectorHandlers);
assert.equal(className, '_11v1eztc');
});
});
});

describe('injectStyleOnce', () => {
it('causes styles to automatically be added', done => {
injectStyleOnce("x", ".x", [{ color: "red" }], false);
Expand Down