Add minify global function to force style names minification

[Soreine]

Fix #288

This exports a global function minify that can be called to control the minification of style names, independently of the NODE_ENV environment value.

Call minify(false) before using StyleSheet.create to never minify style names. Call minify(true) to always minify style names. Defaults to the current behavior of minifying only if process.env.NODE_ENV === 'production'.

[khanbot]

Hey @Soreine,

Thanks for the PR! Mind signing our Contributor License Agreement?

When you've done so, go ahead and comment [clabot:check] and I'll check again.

Yours truly,
khanbot

[Soreine]

[clabot:check]

[khanbot]

CLA signature looks good 👍

[lencioni]

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?

[Soreine]

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

[lencioni]

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

[Soreine]

Update:
I'm sorry, but I don't have the time to dive into performance benchmarks right now. If anyone knows webpack's DefinePlugin and would easily be able to do the benchmarks suggested by @lencioni I would be very grateful.

Maybe another approach would be to measure the number of times this code path is executed in a real-case scenario, then make a separate benchmark that measures the performance of the compiled equivalent of process.env.NODE_ENV === 'production' check versus the compiled runtime check. From these two infos, we can derive a measure of the performance in a real-case scenario.

[dmiller9911]

I went ahead and ran some benchmarks for this. I went ahead and used the same benchmarks that react-native-web has set up. I made 1 change to the benchmark for Aphrodite. I moved StyleSheet.create call into the render function. This resulted in the create call to be called 189,400 times each run. This is a little exaggerated since most apps will have this outside the render function, resulting in a much lower amount of calls. Below are the results of the benchmarks:

All measurements in ms

Current

	DeepTree			WideTree
Run#	Median	Mean	STDEV	Median	Mean	STDEV
1	83.86	91.24	25.45	138.88	137.96	14.68
2	73.11	75.65	17.81	135.88	135.82	16.98
3	75.5	78.28	21.3	124.82	126.94	8.99
4	78.18	81.2	19.11	133.25	132.47	11.36
5	78.02	80.22	23.27	137.2	134.68	12.15
Avg	77.734	81.318	21.388	134.006	133.574	12.832

Fork No Changes

	DeepTree			WideTree
Run#	Median	Mean	STDEV	Median	Mean	STDEV
1	73.26	78.89	18.55	131.28	129.32	11.08
2	78.58	85.81	21.13	133.29	131.41	12.98
3	79.85	83.97	19.84	127.98	128.75	8.65
4	77.73	80.59	20.29	126.12	129.27	14.31
5	81.2	84.5	19.72	121.97	127.1	10.17
Avg	78.124	82.752	19.906	128.128	129.17	11.438

Fork minify(true)

	DeepTree			WideTree
Run#	Median	Mean	STDEV	Median	Mean	STDEV
1	78.01	82.11	20.3	132.83	134.27	16.31
2	76.93	79.11	20.27	128.2	129.77	12.47
3	75.53	78.61	18.68	131.48	132.1	11.27
4	77.79	83.44	24.24	133.77	133.86	11.53
5	63.09	69.78	19.57	125.24	126.04	7
Avg	74.27	78.61	20.612	130.304	131.208	11.716

Fork minify(false)

	DeepTree			WideTree
Run#	Median	Mean	STDEV	Median	Mean	STDEV
1	88.32	90.17	23.77	124.22	132.03	11.17
2	79.62	85	22.64	133.74	130.26	9.82
3	74.24	81.43	22.13	131.51	137.91	14.98
4	79.04	82.64	20.76	128.43	132.51	15.62
5	80.3	84.12	20.23	135.13	135.03	12.08
Avg	80.304	84.672	21.906	130.606	133.548	12.734

There is only a slight difference between each scenario. My guess is if I ran the benchmark more times for each they would end up much closer results. I do not think that having 1 more if statement will really impact performance much, especially inside the StyleSheet.create call.

[lencioni]

@Soreine this seems to fail tests in CI after merging. https://travis-ci.org/Khan/aphrodite/builds/341685191

Any chance you can take a look and open a PR with a fix?

[Soreine]

@lencioni yes, sir.

[Kerumen]

@lencioni Any chance to have this released as an npm tag? Or this library is not maintained anymore?

[Soreine]

@Kerumen this library is still maintained :) I think they just forgot, are busy or are waiting on something

[Kerumen]

@Soreine The latest release was last October (https://github.com/Khan/aphrodite/releases).
We are now in February and more than 30 commits are awaiting a release...

[dmiller9911]

Another release is coming soon I believe. I think the maintainers are just waiting for #300 to go in and then a major release will be created.

[lencioni]

@Kerumen I hope to publish a release soon. This library is maintained and the release has not been forgotten. The next release will be a major version bump, so I want to make sure we get in all of the in-flight breaking changes before we publish.