-
Notifications
You must be signed in to change notification settings - Fork 96
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
Simplify API and introduce automatic routing between models #27
Conversation
Great job! |
``` | ||
|
||
# Install | ||
|
||
```console | ||
```shell |
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 is not "shell". It's "console". Shell is for "#!/usr/bin...." script.
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.
Didn't know there was a difference. I'll update it! 👍
Nice! |
👍 So much better! Looks good to me :) |
If there is a new model you would like to support, or want to add a direct conversion between two existing models, please send us a pull request. | ||
|
||
# License | ||
Copyright © 2011-2016, Heather Arthur. Licensed under the [MIT License](LICENSE). |
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.
You should add yourself here.
Simplify API and introduce automatic routing between models
Landed and published as |
This has been on my list for quite a while now and I finally got around to it.
This PR vastly simplifies this module and makes it focus more on converting rather than clever Javascript functionality.
The goal of this PR was basically to refine the way functions are handled, to reduce the different syntactic ways this module is used (to produce a much cleaner and focused API) - and, probably the most exciting part of it, to automatically route between any conversion possible.
One other subtle, but equally consequential, goal of this PR was to reduce the amount of keys in the returned object. Now, you can be confident that all keys returned by
require('color-convert')
are indeed source color models and not individual functions.I have included four sections to this PR's explanation:
If you don't care about the changes/steps to migrate, skip directly to the Routing portion of this PR.
New Usage
Changes
List of changes (breaking changes denoted with a ✝):
convert()
function. This means you no longer need to (nor can you) call the value returned byrequire('color-convert')
. This also means this library has been boiled down to what is essentially a look-up for conversion functions, and any code relying solely on theconvert()
function should either store the value itself, or switch tocolor
.xxx2yyy()
functions. All functions are now hashed (e.g.xxx.yyy()
).xxx2yyyRaw()
functions have changed toxxx.yyy.raw()
.conversions.js
. This means there are no longer explicit functions for converting between two models via a third model (see the Routing section).ansi
is now known asansi256
-ansi16
remains the same.hex
,keyword
, and theansi
functions) no longer get their arguments grouped into variables. This was previously (and incorrectly) only true forkeyword
andhex
. This is only potentially breaking if you modifiedcolor-convert
directly.Migrating
To migrate to this version if it is merged:
require('color-convert')()
(drop the extra()
).ansi
, toansi256
.xxx2yyy
(e.g.rgb2cmyk
) toxxx.yyy
(e.g.rgb.cmyk
). The changes should be 100% analogous.xxx2yyyRaw()
toxxx.yyy.raw()
. The changes should be 100% analogous.convert()
function. If you really need to store a value and convert it to multiple target color models, either store the value itself and the first part of the hash (e.g.var sourceModel = colorConvert.rgb;
) or switch tocolor
.This means that users only relying on the hashed conversions shouldn't need to do anything (unless you used the
ansi
conversion).Routing
So this is the really fun part of this PR.
Currently, conversion between two models only occurs if there is either a direct conversion (e.g.
rgb
->cmyk
) or an explicit sub-conversion function (e.g.xyz
->rgb
->hex
). If a sub-conversion between two models wasn't created as a function, you couldn't convert between the two.Up until now, this limited parts of this library in two distinct ways:
This is no longer the case. By reducing the amount of 'fluff' code and by using a very quick directed-tree algorithm called Breadth-First Search, the router that is introduced in this PR can create a path from any model, to any model, so long as there is a conversion path between the two.
Further, the router equips each synthetic conversion function it creates with a property called
.conversion
, which is simply an array of strings that describe all of the steps taken to convert the model to another.At the time of this writing, this now allows a conversion such as
lab
->keyword
via routing. Since there is no direct conversion between the two, the router defines the conversion as:You can see these conversion paths via
The router basically creates a composite function that calls
.lab.xyz()
,.xyz.rgb()
,.rgb.keyword()
, in that order, and returns the result. This is same as what happens currently, but built up automatically.The buildup for this is very fast and only happens once (upon the first call to
require()
). This also means future color models need not explicitly define conversions to/from all of the different models unless there is a viable algorithm out there to do so (or in certain cases, there is some sort of optimization between two of them, as is the case ofhsv
->ansi16
).Plugging in optimized conversions now happens automatically and only requires writing an applicable conversion function in
conversions.js
. The router will see that a more direct conversion exists and will adjust/optimize for them accordingly, automatically.Likewise, this also reduces the amount of code in
conversions.js
.This is a rather drastic change and rather than being that BDFL I'd love to get some feedback before I merge this.
// @sindresorhus @MoOx @kevinSuttle @OEvgeny @jbnicolai