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 RGB (256/Truecolor) support #140
Conversation
.travis.yml
Outdated
@@ -1,8 +1,7 @@ | |||
sudo: false | |||
language: node_js | |||
node_js: | |||
- 'stable' |
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 needed. We already define 6 and 4.
index.js
Outdated
@@ -6,9 +6,12 @@ var supportsColor = require('supports-color'); | |||
var defineProps = Object.defineProperties; | |||
var isSimpleWindowsTerm = process.platform === 'win32' && !/^xterm/i.test(process.env.TERM); | |||
|
|||
var levelMapping = ['ansi', 'ansi', 'ansi256', 'ansi16m']; |
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 have ansi
twice there.
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.
The double-ansi is on purpose - if level is 0
we still need to be pulling something from ansi-styles.
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.
Open to suggestions as to how to better approach it. I agree it feels a little clunky. A lot of what chalk does is clunky, though.
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.
I see. A short comment would solve it.
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.
Nvm the Set
comment. I didn't read the code properly.
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.
Done :)
index.js
Outdated
@@ -6,9 +6,12 @@ var supportsColor = require('supports-color'); | |||
var defineProps = Object.defineProperties; | |||
var isSimpleWindowsTerm = process.platform === 'win32' && !/^xterm/i.test(process.env.TERM); | |||
|
|||
var levelMapping = ['ansi', 'ansi', 'ansi256', 'ansi16m']; | |||
var skipModels = ['gray']; |
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.
Did you mean skipModes
?
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.
No - "model" refers to the color models within color-convert. gray
was for CSS gray()
and conflicts with our color gray
or grey
.
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.
Can you add a short comment explaining that. Was not clear when reading the code.
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.
👍
// detect mode if not set manually | ||
this.enabled = !options || options.enabled === undefined ? supportsColor : options.enabled; | ||
// detect level if not set manually | ||
this.level = !options || options.level === undefined ? supportsColor.level : options.level; |
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.
I think we should still support enabled
. Not enough benefit to make such a big breaking change. We can support both. Most will only need to know whether it's enabled or not anyways.
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.
How do we handle when someone supplies {level: 0, enabled: true}
or {level: 2, enabled: false}
?
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.
How you handle mutually exclusive things in general, you throw a user-friendly error, and document it well.
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.
I started making this change and it just doesn't make sense. The code is super hairy - having to check to see if keys are in the configuration object and then do some arbitrary logic on the values. What does enabled: true
mean now? A straight-up level of 1
or do we use supportsColor.level
?
We're already bumping the major version and those advanced users using forced enable/disable outside of the functionality supplied by supports-color
will be willing to adjust how they want Chalk to behave.
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.
I'd vote on leaving enabled
as well, mostly for backwards compatibility reasons.
A straight-up level of 1 or do we use supportsColor.level?
This seems like a good solution. Anything potentially bad with it?
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.
The code you posted is fine, but I think enabled
needs to be defined via Object.defineProperty
and modify/read this.level
under the hood to support that:
chalk.enabled = false;
Please correct me if that's wrong, I may be missing things.
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.
I'm genuinely curious as to how many people actually use the custom constructor?
I'm guessing like you do, but I doubt many people use that. To say the truth, I discovered it just now.
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.
I personally think the code I posted is bloated and huge just to support a property that doesn't really make sense now that there are different levels of support instead of a boolean on/off value.
If we were to do this, then I agree; .enabled
would have to reflect the .level
given a getter/setter descriptor.
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.
I'm only voting for enabled
prop, because I see it somewhat often used in tests to conveniently enable/disable colors when testing output.
Let's see what other guys think.
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.
The level prop will have the same functionality, though :) haha
}); | ||
|
||
ansiStyles.color.closeRe = new RegExp(escapeStringRegexp(ansiStyles.color.close), 'g'); | ||
Object.keys(ansiStyles.color.ansi).forEach(function (model) { |
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.
Use a for-of loop.
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.
Should we change the original loop as well?
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.
Yes, but can do unrelated changes later so not to make this PR too noisy.
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.
I think we should do a complete es2015-ification PR as a separate step to prevent history fragmentation, IMO. But I agree that it should happen.
## 256-colors | ||
## 256/16 million (Truecolor) color support | ||
|
||
Chalk supports 256 colors and, when manually specified, [Truecolor (16 million colors)](https://gist.github.com/XVilka/8346728) on all supported terminal emulators. |
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.
No need for comma after and
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.
Why not? It's a qualifier for the subsequent clause. Grammatically the lack of a comma wouldn't make sense here.
readme.md
Outdated
- `ansi16` | ||
- `ansi256` | ||
- `hcg` | ||
- `apple` |
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.
These needs to be explained, and put the most common ones at the top, like ansi*
and hex
.
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.
ansi*
aren't very common since Chalk handles them already. I'll add hex
to the top.
readme.md
Outdated
|
||
For the methods that support it (listed below), the color will be 'fit' to the color level supported (i.e. RGB colors will be downsampled to 16 colors if only basic support is enabled). | ||
|
||
For a complete list of color models, see [`color-convert`'s list of conversions](https://github.com/Qix-/color-convert/blob/master/conversions.js). Background versions of these models are prefixed with `bg` and the first level of the module capitalized (e.g. `keyword` for foreground colors and `bgKeyword` for background colors). |
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.
Add some examples.
readme.md
Outdated
``` | ||
|
||
Levels are as follows: | ||
|
||
0. All color disabled |
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.
colors
}; | ||
}); | ||
|
||
ansiStyles.color.closeRe = new RegExp(escapeStringRegexp(ansiStyles.color.close), 'g'); |
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.
Why this?
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.
Why not that? Chalk requires a closing code regex for building the style. No sense in re-calculating it for every single color-convert model in the loop right after it since it's the same for all of them.
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.
I mean, it's a little bit dangerous to assign to ansiStyles
as it's a single-ton. So if someone uses ansi-styles
another place, their instance will also be monkey-patched. Might be a non-issue in practise, but wanted to bring it up.
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.
Chalk did it before in the loop above. We can change that, though - I'm not opposed.
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.
Also, it's not applied globally. We fixed that a while back :)
- `ansi16` | ||
- `ansi256` | ||
- `hcg` | ||
- `apple` (see [qix-/color-convert#30](https://github.com/Qix-/color-convert/issues/30)) |
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.
I think one example per color model would be very helpful. Show how a single color is represented through all the various methods. Maybe through a table that contains chalk method | model name | example code
.
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.
Most of these color models are not very practically useful though. I doubt most of these will be used by users. I would prefer to only document the most popular ones and rather leave it to color-convert to document the rest.
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.
The other thing is that the way color-convert is versioned all new models that are added 'just work' with whatever is using them assuming you're using ^
. This would be an ever changing list.
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.
I'm fine with just documenting the most popular ones. I think these are rgb,hex,keyword,hsl,cymk. We can link to the color-convert docs for further reading.
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.
@silverwind 👍 Except for cmyk
. I don't see why anyone would use that in the terminal. CMYK is for printing.
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.
So what should be put there, then? Just a basic description? Or a line of sample code?
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.
Expand the examples to include above 4 using the same color. After that, I guess some work is needed on color-convert
's readme to have more examples for the rarely used models. After that, link to that readme (instead of linking to the code).
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.
Yeah, I need to start enforcing readme updates in color-convert
.
Can do.
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.
How's that?
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.
Examples looking good. I guess it's cleaner to remove the models without examples once color-convert's has examples for those.
readme.md
Outdated
|
||
Chalk supports 256 colors and, when manually specified, [Truecolor (16 million colors)](https://gist.github.com/XVilka/8346728) on all supported terminal emulators. | ||
|
||
For the methods that support it (listed below), the color will be 'fit' to the color level supported (i.e. RGB colors will be downsampled to 16 colors if only basic support is enabled). |
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.
"For supported methods"
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.
👍
|
||
ansiStyles.color.closeRe = new RegExp(escapeStringRegexp(ansiStyles.color.close), 'g'); | ||
Object.keys(ansiStyles.color.ansi).forEach(function (model) { | ||
if (skipModels.indexOf(model) !== -1) { |
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.
skipModels.indexOf(model) >= 0
would look nicer, imo.
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.
I tend to err on !== -1
since the spec specifically states
The first index of the element in the array; -1 if not found.
It's obviously bike shedding. It'd be nice if XO made a definitive decision on it.
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.
True that.
|
||
ansiStyles.bgColor.closeRe = new RegExp(escapeStringRegexp(ansiStyles.bgColor.close), 'g'); | ||
Object.keys(ansiStyles.bgColor.ansi).forEach(function (model) { | ||
if (skipModels.indexOf(model) !== -1) { |
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.
readme.md
Outdated
|
||
Chalk supports 256 colors and, when manually specified, [Truecolor (16 million colors)](https://gist.github.com/XVilka/8346728) on all supported terminal emulators. | ||
|
||
For supported methods (listed below), the color will be 'fit' to the color level supported (i.e. RGB colors will be downsampled to 16 colors if only basic support is enabled). |
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.
perhaps will be 'fit'
=> will be converted
?
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.
I think downsampled
was the word I was looking for here.
E.g. if your support level is 2 (256 colors) and you give it an RGB, it will downsample the full 16 million color down to the 256-palette color.
Is downsample
correct here? I want to be a bit more specific than convert
since in this context it could be mistaken as converting to another color model (which is technically true, I suppose...)
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.
Yep, this is better, but downsampled
is in the parenthesis as well. Thinking whether it'd read a bit repetitive.
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.
Colors are downsampled from 16 million RGB values to an ANSI color format that is supported by the terminal emulator (or by specifying
{level: n}
as a chalk option). For example, Chalk configured to run at level 1 (basic color support) will downsample an RGB value of#FF0000
(red) to31
(ANSI escape for red).
Is that closer? It's such awkward verbiage :|
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.
Oh yes, I think this is better and more detailed ;)
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.
Cool, thanks :D
readme.md
Outdated
|
||
Some examples: | ||
|
||
For a complete list of color models, see [`color-convert`'s list of conversions](https://github.com/Qix-/color-convert/blob/master/conversions.js). Background versions of these models are prefixed with `bg` and the first level of the module capitalized (e.g. `keyword` for foreground colors and `bgKeyword` for background colors). |
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.
I'd move this paragraph below the examples.
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.
Agreed. Does that look better?
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.
Yes, that's fine.
Great work, @Qix-! |
@Qix- Seems as though this issue was introduced in If I revert back to what is installed by |
It's not malformed; it's a lambda that returns a lambda. Not sure when that got changed but it's valid. What's the error message? Note that you need Node >= 4 or you need to babel it. |
I am using Node 6.8.1 If I completely remove chalk (which removes ansi-styles) and reinstall chalk 1.1.3 Then install the version you outlined, all works. However, if I then remove node_modules and reinstall, ansi-styles 3.0 is installed and webpack again fails |
@qix sorry to be sort of sparse, replying from phone |
@qix and I am using babel (via babel-loader in webpack) |
What exactly is the error message being produced? Which tool is producing it? And what can I do to reproduce it on my end? |
@qix I will try and create a small reproducible example. |
@Qix- OK, I have created a reproducible archive and will post the repo shortly. |
@Qix- OK, here you go. Here is a repo which outlines the issue |
@mikeerickson Thank you very much - I can reproduce. It's an UglifyJS bug where it doesn't quite fully support ES6 yet. Check out mishoo/UglifyJS#448. EDIT: Relevant UglifyJS changes that are planned to fix this. It is indeed the part of the AST that handles blocks that is not up to date. |
@mikeerickson Run this: rm -rf node_modules
npm i
find node_modules -type d | grep uglify-js | xargs rm -rf
patch ./node_modules/webpack/package.json <<EOF
70c70
< "uglify-js": "~2.7.3",
---
> "uglify-js": "mishoo/UglifyJS2#harmony",
EOF
npm i
npm i chalk/chalk#256-truecolor
node ./index.js
npm run build Obviously a temporary workaround for now, but the UglifyJS people are really dragging their heels on this one. Fortunately it looks as if webpack has pretty updated dependencies so whenever UglifyJS finally gets around to merging their |
What a shame that we still don't have a ES6 minifier at this day and age 😢. |
Hello, this is wonderful, but is the PR abandoned? Or do you need any help in bringing it further? |
I'm looking forward to this one too; is this still in the works? |
@sindresorhus ping? Also, +1 for |
Yay! At last. Thanks for working on this @Qix-. Sorry about the delay everyone. |
🎉 Great work @Qix- |
Finally closes #73.
References #20, #35, #37, #57, #87, #126. We know some of you have been waiting for this one for a very long time.
This PR is blocked by a few remaining issues over at
supports-color
. @sindresorhus has promised me he will take a look at those and make some executive decisions within the coming week. After they are resolved, its version will be bumped in this PR and, pending approval from the community, this will be merged and released.This is an additive release - however, please see the breaking changes section below for a reason why it's a major release.
This adds two new suites of chalk-able modes:
color-convert
models as names (e.g.chalk.rgb(123, 45, 67)
orchalk.keyword('orange')
)bg
prefix, (e.g.chalk.bgRgb(123, 45, 67)
orchalk.bgKeyword('orange')
)All RGB color functions in turn return your usual chalk builder. Some examples:
chalk.hex('#DEADED').underline('Hello, world!')
chalk.keyword('orange')('Some orange text')
chalk.rgb(15, 100, 204).inverse('Hello!')
This is per @silverwind's suggestion in #73, which we agreed is the most intuitive way to approach this API.
This module now requires Node.js 4 or above. This is why it is a major release bump.
The only potentially API-related breaking changes is if you use Chalk's
.enabled
property, you'll need to replace it with.level
, which is now an integer (where0
indicates not enabled and anything >= 1 indicates enabled, and further, one of several levels of color granularity).As well, if you're passing an option to chalk to forcefully set whether or not it's enabled, you'll need to change:
{enabled: false}
->{level: 0}
, or{enabled: true}
->{level: 1}
(or whatever level you'd like - level 1 is equivalent to Chalk's current level of support).The levels, as described in
supports-color
, are:Please don't hesitate to give me feedback on the new API.
Thanks for everyone's continued patience - let this be a celebration of some of the first OSS contributions I made here on Github, and let it serve as a relic of some of the great connections I've made through it. 🦄
cc @sindresorhus @jbnicolai @dthree @silverwind @stevemao @arthurvr @mikeerickson @rektide @mattbasta, as well as @XVilka (Thank you for your research on support! It has proven invaluable in many ways.)
Thanks everyone!