Swap chalk for turbocolor

[jorgebucaran]

Summary

This PR swaps chalk for turbocolor. It ought to give us a humble perf boost as turbocolor loads >20x faster and applies styles >18x faster than chalk. Terminal color detection is also built-in, so I was able to simplify the code in one place.

I let some comments in the diff to explain a few other things.

Let me know if this looks good to you and if you'd like to take my contribution.

turbocolor (1.1kB) / chalk (20.2kB)

# Load Time
chalk: 15.190ms
turbocolor: 0.777ms

# All Colors
chalk × 8,729 ops/sec
turbocolor × 158,383 ops/sec

# Chained Colors
chalk × 1,838 ops/sec
turbocolor × 39,830 ops/sec

# Nested Colors
chalk × 4,049 ops/sec
turbocolor × 59,833 ops/sec

Cheers! 👋

@Rich-Harris @lukastaegert @TrySound

[jorgebucaran]

This check is built-into turbocolor, so we don't need it here.

Note: One can still force color support on or off using tc.enabled: boolean.

[lukastaegert]

Looks good to me. Will need to play around with it to see if I encounter any unexpected issues when run in different environments but code-wise, this looks fine!

[lukastaegert]

Also 👍 for the reduced size!

[lukastaegert]

More precisely, the rollup CLI shrank from 107KB to 67KB 🎉 (but of course it is not minified or zipped). So basically, chalk was a third of the code of our CLI 😱

It seems the colors are a little off, though. Basically, they are much brighter than before which reduces legibility especially of yellow, cyan and green status texts on a terminal with a white background. Do you think something can be done about this?

(First: chalk, second: TurboColor)

[lukastaegert]

It seems the issue is that turbocolor only knows the bright versions of the colors which also means that turbocolor is not a 1-to-1 drop-in replacement for chalk here. Any chance you will add non-bright color support?

Legibility is suffering from bright color use

[jorgebucaran]

@lukastaegert Thanks, good catch! I've updated the PR and published a fix jorgebucaran/colorette@a0cfbec

Any chance you will add non-bright color support?

Default/non-bright colors is the default now.

[lukastaegert]

👍

[styfle]

Awesome! It looks like turbocolor is significantly smaller than chalk too 👍

Name	Install Size
turbocolor@3.0.0
chalk@2.4.1

[jonschlinkert]

Hey guys, thought you might be interested to know that "turbocolor" has a serious memory leak. I know, because when @jorgebucaran took the code that I wrote for a PR that I submitted to @doowb's ansi-colors, and then, in an attempt to make his code look different and perform better in benchmarks, he removed, among other things, the part of the code that prevented the memory leak from happening. I spent a great deal of time thinking about how to get that code working using something like 1/10th of the code used by chalk. Getting the color stack to clear correctly was a real pain. @jorgebucaran my recommendation is that you do the same. Spend time figuring out how to solve problems, not on how to make my code look like you wrote it. I've been doing this a while, and there are exceedingly rare situations in which I would feel compelled to speak up about something like this, and this is one of them.

Apologies to the rollup team for the intrusion and the drama. I won't continue this discussion here, so we can keep spirits high and stay focused on solving issues. I would be happy to answer questions or provide more detail if you want to contact me directly.

[shellscape]

FWIW I'll be introducing a new logger sometime soon that leverages some of chalk's feature set. While it's a few KB larger, nothing else comes close in terms of platform support and features. I wasn't able to weigh in on this PR before it was merged, but I'll be opening up discussion for a reversion. I also don't have any skin in the game as to the alleged plagiarism, only a personal preference of lib.

@jonschlinkert objectively speaking, it would be helpful if you could highlight the part of the code that prevents the memory leak, that which when removed results in a speed increase. That in of itself would warrant reverting.

@lukastaegert I believe this is worth a second look.

[jorgebucaran]

@jonschlinkert Hi! I didn't know you, until now. This is the first time we interact together. I'm sorry you feel that I plagiarized your code. I did not. I'd never claim as mine something that isn't. Looks like this is the PR you may be talking about. This is the first time I see it.

Turbocolor (previously clorox/clor) has been around since May, 2015. The basic gist was here. I did know about ansi-colors (and kleur too) and thoughtfully credited the authors in my release notes for turbocolor@1.0.0.

Clorox has been renamed to Turbocolor to reflect recent performance improvements. The speed boost can be largely credited to the work of Brian Woodward and Luke Edwards in ansi-colors and kleur, respectively.

I credit them for using setPrototypeOf/__proto__, which I originally learned from kleur. I didn't know you had contributed that code. For that reason, I just added you to the credits in the notes.

---

@lukastaegert @shellscape I haven't noticed any memory problems, or problems with each individual color function's stack not being cleared. If that was the case, it would be visible in the nested color tests. I'll look more into it.

[lukastaegert]

I would definitely be interested in learning more about the potential memory issues. Otherwise I still prefer a targeted library to the universal solution chalk offers as it is packed with features we will never use and this code really seems to resist tree-shaking and we only use some colors + bold/underline anyway.

Actually the most performant solution with the least overhead would be to just inline functions that add the few bits of formatting we need. Beyond that, as this is not really performance critical and I would not want to look up ANSI codes each time we have a new idea, something targeted like turbocolor or the other libraries mentioned seems about right to me.

[lukastaegert]

BTW interesting that __proto__ seems to improve performance considering it comes from Pandora's box of performance hazards: https://developer.mozilla.org/en-US/docs/Web/JavaScript/The_performance_hazards_of__%5B%5BPrototype%5D%5D_mutation

[jorgebucaran]

@lukastaegert I looked more into it and I didn't find anything wrong with the internal stack array or with anything else. In turbocolor we keep a stack for every style function and clear it by popping all the ansi codes when the style function is invoked. We use the popped items to assemble the output stylized string.

https://github.com/jorgebucaran/turbocolor/blob/c64ce012559d91aba7ddf0304ae39edb94d03588/index.js#L28-L30

I didn't want to come back empty-handed, so I did a bit of research, and found leakage, a package that helps detect memory leaks and added a test to check against leaks.

https://github.com/jorgebucaran/turbocolor/blob/f5a8b02f5f7614e49af6cb0e7af1f6ec7201b992/test/index.js#L3-L23

Travis Status

[jorgebucaran]

@lukastaegert @shellscape Howdy! I am adding my comment here because @jonschlinkert has apparently blocked me and that prevents me from replying directly to him, and as a result, I can't post on #2412.

Following is my reply to each point he is making on the PR.

ansi-colors is more performant than turbocolor

The load time test is wrong. It always favors the last module that is required. Turbocolor loads under 0.7ms in my benchmarks too, but only because I am requiring it last. I didn't realize that until now, so I'm going to investigate how to properly test load time or just remove that test until I know how to accurately and fairly do it.

The rest of the benchmark results (theirs and mine) show that turbocolor is faster in every test except in the nested test. Why? As of ansi-colors@3.0.0 they're failing to remove the close escape code when building the output string here. This means you can't nest colors like this.

console.log(red(`red ${green("green") not red`))

I could do the same in turbocolor to get those extra ops.

ansi-colors correctly renders chained styles. turbocolor has a rendering bug that causes the wrong color to be rendered when chained colors are assigned to a variable - the is easily reproducible and demonstrated below.

In turbocolor we can do this instead:

const redBgCyanBold = s => tc.red.bgCyan.bold(s)

console.log(redBgCyanBold("Hello"))

This is how I've always done it since clorox and chalk before that. The docs don't say you can assign a color chain to a variable, therefore I don't consider it to be a bug. Now the real question: is this a feature we want? It's not difficult to add, but it will affect performance.

BTW this issue was filed and discussed in jorgebucaran/colorette#21.

(technically it's a memory leak...

The internal stack is always cleared, that's why assigning a color chain to a variable doesn't keep the stack. If it did, it would be a memory leak, though. So, this can't be classified as a memory leak.

ansi-colors correctly renders background colors. turbocolor renders "bright background" colors as (normal) background colors, but inconsistently, as some are bright and some are not, which is confusing, unexpected and incorrect.

turbocolor swaps bright-background colors for background colors. (surprise! turbocolor gives users unexpected colors in the terminal!)

This is outdated. @lukastaegert brought this to my attention in #2339 (comment) and it's been fixed for a while alright. See: jorgebucaran/colorette@a0cfbec

turbocolor fails half of the ansi-colors unit tests (chalk passes them all)

Turbocolor doesn't have every chalk feature, e.g., 256/Truecolor color support, a tagged function, etc. Turbocolor is a targeted library, as opposed to a universal, multi-featured library like chalk. Having said that, chalk would also fail turbocolor's unit tests! The end result is exactly the same though. 🎉

The basic feature set is there and turbocolor can be used as a decent "drop in" replacement for chalk that will work for most cases as shown by this PR.

turbocolor does not support bright or bright-background colors

This is 100% true! Turbocolor only exports normal colors escape codes at the moment. 😄

[shellscape]

@jorgebucaran it seems that Rollup has been made the center of a sort of feud. That's not exactly a place where we want to be. I think you've made your case very eloquently and we certainly appreciate all of the data you've provided. It's obvious that you and @jonschlinkert are passionate about this particular space.

We'll take all of the information into consideration for choosing which library ships. Please do understand that the decision we make will not be one that favors (or promotes) one library over another, nor one author over another. It will be a decision that maintainers determine is best for the project and its future needs.

I would ask that you let this topic of discussion be for now and allow the maintainers to focus our energies on other aspects of the library, lest this feed the feud and turn into endless bike-shedding. If there are comparisons or challenges you'd like to make towards other libraries, please do so on their respective repositories. Unfortunately third-party repos aren't the right place for that.

@lukastaegert I believe it would be prudent to lock this PR to contributors for a short time.