Reimplement color processing #5529

vjeux · 2016-01-25T06:25:08Z

Summary:

Problem:

As I was trying to document what color formats we supported, I realized that our current implementation based on the open source project tinycolor supported some crazy things. A few examples that were all valid:

tinycolor('abc')
tinycolor(' #abc ')
tinycolor('##abc')
tinycolor('rgb 255 0 0')
tinycolor('RGBA(0, 1, 2)')
tinycolor('rgb (0, 1, 2)')
tinycolor('hsv(0, 1, 2)')
tinycolor({r: 10, g: 10, b: 10})
tinycolor('hsl(1%, 2, 3)')
tinycolor('rgb(1.0, 2.0, 3.0)')
tinycolor('rgb(1%, 2%, 3%)')

The integrations of tinycolor were also really bad. processColor added "support" for pure numbers and an array of colors!?? ColorPropTypes did some crazy trim().toString() and repeated a bad error message twice.

Solution:

While iteratively cleaning the file, I eventually ended up reimplementing it entierly. Major changes are:

The API is now dead simple: returns null if it doesn't parse or returns the int32 representation of the color
Stricter parsing of attributes. Instead of allowing all the arguments to be numbers with or without percentages, now you need to specify the correct type between integer, number and percentage.
The internals only work on int32 numbers, there's no longer tons of object creations and copy
The color map is now storing the computed value directly instead of parsing it over and over again
Made the wrapper functions just delegate to tinycolor instead of implementing diverging logic
Added unit tests on the allowed and not allowed formats
Flowified the file
Fixed a bug where hsl values with s === 0 would return the wrong result

I'm not sure how many times this function is being called but we were allocating 5 (!!!!) rgba objects for each call and doing a lot of expensive string parsing. The new implementation should be much faster.

Risk:

I'm confident that the values that were supported before still work fine. But there may be cases where people used an unsupported variant that is now going to redbox in dev and ignore the color in prod. I think that it's worth doing as what was supported was really clowny.

Test Plan:
Unit tests for now. I want to gather feedback and if people are happy with it I'll push it forward.

facebook-github-bot · 2016-01-25T06:25:39Z

By analyzing the blame information on this pull request, we identified @javache, @nicklockwood and @a2 to be potential reviewers.

vjeux · 2016-01-25T06:27:01Z

Libraries/Animated/src/__tests__/Interpolation-test.js

-    expect(interpolation(0)).toBe('rgba(100, 120, 140, 0.5)');
-    expect(interpolation(0.5)).toBe('rgba(117.5, 186, 126, 0.75)');
+    expect(interpolation(0)).toBe('rgba(100, 120, 140, 0.4)');
+    expect(interpolation(0.5)).toBe('rgba(117.5, 186, 126, 0.7)');


Needed to change those as we are now only having 8 bits of precision. 0.75 * 255 = 191.25 is not a round number. It doesn't really change anything, the platform unlikely supports more than 8 bits of precision for the alpha channel.

vjeux · 2016-01-25T06:43:14Z

Libraries/vendor/tinycolor/tinycolor.js

-    s = bound01(s, 100);
-    l = bound01(l, 100);
-
-    function hue2rgb(p, q, t) {


no need to keep it inside anymore (we have private scope with commonjs modules)

ide · 2016-01-25T07:55:57Z

Can you move this out of vendor if it's a new implementation?

ide · 2016-01-25T08:01:24Z

Libraries/vendor/tinycolor/tinycolor.js

+
+  var match;
+
+  // Ordered based on occurences on Facebook codebase


spelling: occurrences
also basing off of frequency is smart

nicklockwood · 2016-01-25T09:26:35Z

I was planning to optimize this, so thanks for saving me the trouble!

You've removed a few deliberately supported features (#rgba and color arrays) along with the crud though, so we'll need to restore those.

Also, I think it should probably be renamed to something other than 'tinycolor', since it no longer shares any heritage with the original lib.

vjeux · 2016-01-25T12:47:09Z

Thanks for the very good review! I'll address all the feedback and rename the file/move it somewhere else :)

Summary: **Problem:** As I was trying to document what color formats we supported, I realized that our current implementation based on the open source project tinycolor supported some crazy things. A few examples that were all valid: ``` tinycolor('abc') tinycolor(' #abc ') tinycolor('##abc') tinycolor('rgb 255 0 0') tinycolor('RGBA(0, 1, 2)') tinycolor('rgb (0, 1, 2)') tinycolor('hsv(0, 1, 2)') tinycolor({r: 10, g: 10, b: 10}) tinycolor('hsl(1%, 2, 3)') tinycolor('rgb(1.0, 2.0, 3.0)') tinycolor('rgb(1%, 2%, 3%)') ``` The integrations of tinycolor were also really bad. processColor added "support" for pure numbers and an array of colors!?? ColorPropTypes did some crazy trim().toString() and repeated a bad error message twice. **Solution:** While iteratively cleaning the file, I eventually ended up reimplementing it entierly. Major changes are: - The API is now dead simple: returns null if it doesn't parse or returns the int32 representation of the color - Stricter parsing of attributes. Instead of allowing all the arguments to be numbers with or without percentages, now you need to specify the correct type between integer, number and percentage. - The internals only work on int32 numbers, there's no longer tons of object creations and copy - The color map is now storing the computed value directly instead of parsing it over and over again - Made the wrapper functions just delegate to tinycolor instead of implementing diverging logic - Added unit tests on the allowed and not allowed formats - Flowified the file - Fixed a bug where hsl values with s === 0 would return the wrong result **Risk:** I'm confident that the values that were supported before still work fine. But there may be cases where people used an unsupported variant that is now going to redbox in dev and ignore the color in prod. I think that it's worth doing as what was supported was really clowny. Test Plan: Unit tests for now. I want to gather feedback and if people are happy with it I'll push it forward.

facebook-github-bot · 2016-01-27T22:42:22Z

@vjeux updated the pull request.

vjeux · 2016-01-27T22:43:02Z

@facebook-github-bot import

vjeux · 2016-01-27T22:43:30Z

Libraries/Animated/src/Interpolation.js

 'use strict';

-var tinycolor = require('tinycolor');
+var normalizeColor = require('normalizeColor');


I renamed it to normalizeColor

facebook-github-bot · 2016-01-27T22:43:50Z

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/929606970420579/int_phab to review.

vjeux · 2016-01-27T22:51:23Z

The error is pretty awesome

vjeux · 2016-01-27T22:54:51Z

Grrr, import doesn't trigger tests for some reasons :( Would love a review so that I can to try and shipit

ide · 2016-01-28T08:34:15Z

LGTM

vjeux · 2016-01-28T14:54:31Z

@facebook-github-bot shipit

facebook-github-bot · 2016-01-28T14:55:28Z

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/929606970420579/int_phab to review.

ide · 2016-01-28T19:29:45Z

If you have internal TTI profiling tools that show the before/after effects of this diff I'd be interested in seeing them since this should speed up the first render() pass especially.

nicklockwood · 2016-01-28T21:04:19Z

@ide the results show a slight improvement, but below the threshold of confidence for the accuracy of our tests ¯_(ツ)_/¯

ide · 2016-01-28T21:14:08Z

@nicklockwood thanks for the report. A slight improvement is better than nothing =)

Summary: **Problem:** As I was trying to document what color formats we supported, I realized that our current implementation based on the open source project tinycolor supported some crazy things. A few examples that were all valid: ``` tinycolor('abc') tinycolor(' #abc ') tinycolor('##abc') tinycolor('rgb 255 0 0') tinycolor('RGBA(0, 1, 2)') tinycolor('rgb (0, 1, 2)') tinycolor('hsv(0, 1, 2)') tinycolor({r: 10, g: 10, b: 10}) tinycolor('hsl(1%, 2, 3)') tinycolor('rgb(1.0, 2.0, 3.0)') tinycolor('rgb(1%, 2%, 3%)') ``` The integrations of tinycolor were also really bad. processColor added "support" for pure numbers and an array of colors!?? ColorPropTypes did some crazy trim().toString() and repeated a bad error message twice. **Solution:** While iteratively cleaning the file, I eventually ended up reimplementing it entierly. Major changes are: - The API is now dead simple: returns null if it doesn't parse or returns the int32 representation of the color - Stricter parsing of at Closes facebook#5529 Reviewed By: svcscm Differential Revision: D2872015 Pulled By: nicklockwood fb-gh-sync-id: df78244eefce6cf8e8ed2ea51f58d6b232de16f9

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jan 25, 2016

vjeux reviewed Jan 25, 2016
View reviewed changes

ide reviewed Jan 25, 2016
View reviewed changes

vjeux force-pushed the color branch from 311f54e to 7ee276c Compare January 27, 2016 22:42

vjeux reviewed Jan 27, 2016
View reviewed changes

ghost closed this in c8a0a3e Jan 29, 2016

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplement color processing #5529

Reimplement color processing #5529

vjeux commented Jan 25, 2016

facebook-github-bot commented Jan 25, 2016

vjeux Jan 25, 2016

vjeux Jan 25, 2016

ide commented Jan 25, 2016

ide Jan 25, 2016

vjeux Jan 25, 2016

nicklockwood commented Jan 25, 2016

vjeux commented Jan 25, 2016

facebook-github-bot commented Jan 27, 2016

vjeux commented Jan 27, 2016

vjeux Jan 27, 2016

facebook-github-bot commented Jan 27, 2016

vjeux commented Jan 27, 2016

vjeux commented Jan 27, 2016

ide commented Jan 28, 2016

vjeux commented Jan 28, 2016

facebook-github-bot commented Jan 28, 2016

ide commented Jan 28, 2016

nicklockwood commented Jan 28, 2016

ide commented Jan 28, 2016


		var match;

		// Ordered based on occurences on Facebook codebase

Reimplement color processing #5529

Reimplement color processing #5529

Conversation

vjeux commented Jan 25, 2016

facebook-github-bot commented Jan 25, 2016

vjeux Jan 25, 2016

Choose a reason for hiding this comment

vjeux Jan 25, 2016

Choose a reason for hiding this comment

ide commented Jan 25, 2016

ide Jan 25, 2016

Choose a reason for hiding this comment

vjeux Jan 25, 2016

Choose a reason for hiding this comment

nicklockwood commented Jan 25, 2016

vjeux commented Jan 25, 2016

facebook-github-bot commented Jan 27, 2016

vjeux commented Jan 27, 2016

vjeux Jan 27, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Jan 27, 2016

vjeux commented Jan 27, 2016

vjeux commented Jan 27, 2016

ide commented Jan 28, 2016

vjeux commented Jan 28, 2016

facebook-github-bot commented Jan 28, 2016

ide commented Jan 28, 2016

nicklockwood commented Jan 28, 2016

ide commented Jan 28, 2016