Skip to content
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

Swap chalk for turbocolor. #544

Closed
wants to merge 1 commit into from
Closed

Swap chalk for turbocolor. #544

wants to merge 1 commit into from

Conversation

jorgebucaran
Copy link

Hello webpack-cli maintainers! 👋

This PR basically swaps chalk for Turbocolor.


What kind of change does this PR introduce?

Swaps one library for another that uses the same API, but it's lighter (no deps) and has significantly better load/runtime performance (~>20x faster).

Did you add tests for your changes?

Same tests pass, so should be good.

If relevant, did you update the documentation?

I did, is it good?

Summary

It should give you a small perf boost as turbocolor loads >20x faster and applies styles >18x faster than chalk for the same API (see benchmarks). Node.js >=v4 supported.

# 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

Does this PR introduce a breaking change?
No!


Let me know if this is acceptable to you and if I need to change anything. Cheers!

@jsf-clabot
Copy link

jsf-clabot commented Jul 14, 2018

CLA assistant check
All committers have signed the CLA.

@webpack-bot
Copy link

@jorgebucaran The most important CI builds failed. This way your PR can't be merged.

Please take a look at the CI results from travis (failure) and fix these issues.

@evenstensberg
Copy link
Member

Hi @jorgebucaran, thanks for the attention and PR!

I'm going to ask the team first, let me get back to you once we're decided!

@jorgebucaran
Copy link
Author

jorgebucaran commented Jul 14, 2018

Hi @ev1stensberg, thanks sounds good :)

@ematipico
Copy link
Contributor

Any comparison regarding the sizes of the packages?

@jorgebucaran
Copy link
Author

jorgebucaran commented Jul 14, 2018

@ematipico I usually go to bundlephobia to find out:

@evenstensberg
Copy link
Member

@jorgebucaran any difference between the two packages? API wise ?

@jorgebucaran
Copy link
Author

jorgebucaran commented Jul 15, 2018

@ev1stensberg Feature-wise chalk has more options, but webpack-cli is using none of them and some of those options are not widely supported by terminals. API-wise, they're virtually the same, but let's see one thing.

Chalk supports variadic arguments like:

console.log(chalk.red("Hello", "World"))

...whereas turbocolor is a unary function by design. (There are so many ways that arguments can cause a function to be unoptimizable.)

console.log(color.red("Hello" + "World"))

// or 

console.log(color.red("Hello %s"), "World")

// or

const name = "World"
console.log(color.red(`Hello ${name}`))

You can see all the features supported by turbocolor in the usage section.


Hope that answers your question :)

@webpack-bot
Copy link

Hi @jorgebucaran.

Just a little hint from a friendly bot about the best practice when submitting pull requests:

Don't submit pull request from your own master branch. It's recommended to create a feature branch for the PR.

You don't have to change it for this PR, just make sure to follow this hint the next time you submit a PR.

@evenstensberg
Copy link
Member

Hi @jorgebucaran . Me and my team have discussed a bit, and we don't really see the point of swapping chalk for another library. Chalk is stable and good enough for us as of this date. Thanks for the PR 👍

@jorgebucaran
Copy link
Author

@ev1stensberg No problem! Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants