Skip to content
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

Add minify global function to force style names minification #291

Merged
merged 2 commits into from
Feb 15, 2018
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
23 changes: 20 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
# Aphrodite: Inline Styles that work

[![npm version](https://badge.fury.io/js/aphrodite.svg)](https://badge.fury.io/js/aphrodite)
[![Build Status](https://travis-ci.org/Khan/aphrodite.svg?branch=master)](https://travis-ci.org/Khan/aphrodite)
[![Coverage Status](https://coveralls.io/repos/github/Khan/aphrodite/badge.svg?branch=master)](https://coveralls.io/github/Khan/aphrodite?branch=master)
[![npm version](https://badge.fury.io/js/aphrodite.svg)](https://badge.fury.io/js/aphrodite)
[![Build Status](https://travis-ci.org/Khan/aphrodite.svg?branch=master)](https://travis-ci.org/Khan/aphrodite)
[![Coverage Status](https://coveralls.io/repos/github/Khan/aphrodite/badge.svg?branch=master)](https://coveralls.io/github/Khan/aphrodite?branch=master)
[![Gitter chat](https://img.shields.io/gitter/room/Khan/aphrodite.svg)](https://gitter.im/Khan/aphrodite)

[![gzip size][gzip-badge]][unpkg-dist]
Expand Down Expand Up @@ -185,6 +185,23 @@ to avoid this behaviour, then instead of importing `aphrodite`, import
import { StyleSheet, css } from 'aphrodite/no-important';
```

## Minifying style names

By default, Aphrodite will minify style names down to their hashes in production
(`process.env.NODE_ENV === 'production'`). You can override this behavior by
calling `minify` with `true` or `false` before calling `StyleSheet.create`.

This is useful if you want to facilitate debugging in production for example.

```js
import { StyleSheet, minify } from 'aphrodite';

// Always keep the full style names
minify(false);

// ... proceed to use StyleSheet.create etc.
```

## Font Faces

Creating custom font faces is a special case. Typically you need to define a global `@font-face` rule. In the case of Aphrodite we only want to insert that rule if it's actually being referenced by a class that's in the page. We've made it so that the `fontFamily` property can accept a font-face object (either directly or inside an array). A global `@font-face` rule is then generated based on the font definition.
Expand Down
19 changes: 18 additions & 1 deletion src/exports.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,26 @@ type Extension = {
export type MaybeSheetDefinition = SheetDefinition | false | null | void
*/

// True to minify classnames.
// False to not minify classnames.
// Unset to minify only in 'production' environment
let forceMinify = undefined;
const shouldMinify = () => {
if (forceMinify !== undefined) {
return forceMinify;
} else {
return process.env.NODE_ENV === 'production';
}
}


const StyleSheet = {
create(sheetDefinition /* : SheetDefinition */) {
return mapObj(sheetDefinition, ([key, val]) => {
const stringVal = JSON.stringify(val);
return [key, {
_len: stringVal.length,
_name: process.env.NODE_ENV === 'production' ?
_name: shouldMinify() ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

With the previous version of this code, the conditional will end up being minified out in production, which gives us a small performance boost. Switching this to a runtime check will affect performance some amount.

It is probably a pretty small hit, but given that this code is in a pretty performance-sensitive path, it is definitely worth profiling the fully simplified version (e.g. _name: hashString(stringVal)) against this version in real-world scenarios so we at least understand what the performance impact of this change is.

Can you do this profiling and post your findings on this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's interesting... Do you have ready to use profilers ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not that I am aware of. I think to try this out, it would probably be best to use npm link in a project that is setting the NODE_ENV environment variable to production and minifying it out properly (e.g. in conjunction with webpack's DefinePlugin).

hashString(stringVal) : `${key}_${hashString(stringVal)}`,
_definition: val
}];
Expand Down Expand Up @@ -131,6 +144,10 @@ const makeExports = (
StyleSheetServer,
StyleSheetTestUtils,

minify(shouldMinify /* : boolean */) {
forceMinify = shouldMinify;
},

css(...styleDefinitions /* : MaybeSheetDefinition[] */) {
return injectAndGetClassName(
useImportant, styleDefinitions, selectorHandlers);
Expand Down
71 changes: 71 additions & 0 deletions tests/index_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
StyleSheet,
StyleSheetServer,
StyleSheetTestUtils,
minify,
css
} from '../src/index.js';
import { reset } from '../src/inject.js';
Expand Down Expand Up @@ -257,6 +258,76 @@ describe('StyleSheet.create', () => {
})
});

describe('minify', () => {
describe('true', () => {
beforeEach(() => {
minify(true);
});

afterEach(() => {
minify(undefined);
});

it('minifies style names', () => {
const sheet = StyleSheet.create({
test: {
color: 'red',
height: 20,

':hover': {
color: 'blue',
width: 40,
},
},
});

assert.equal(sheet.test._name, 'j5rvvh');
});
})

describe('false', () => {
beforeEach(() => {
minify(false);
});

afterEach(() => {
minify(undefined);
});

it('does not minifies style names', () => {
const sheet = StyleSheet.create({
test: {
color: 'red',
height: 20,

':hover': {
color: 'blue',
width: 40,
},
},
});

assert.equal(sheet.test._name, 'test_j5rvvh');
});

it('does not minifies style names, even with process.env.NODE_ENV === \'production\'', () => {
const sheet = StyleSheet.create({
test: {
color: 'red',
height: 20,

':hover': {
color: 'blue',
width: 40,
},
},
});

assert.equal(sheet.test._name, 'test_j5rvvh');
});
})
});

describe('rehydrate', () => {
beforeEach(() => {
global.document = jsdom.jsdom();
Expand Down