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

Replace chalk with nanocolors #13783

Closed
wants to merge 11 commits into from
Closed

Replace chalk with nanocolors #13783

wants to merge 11 commits into from

Conversation

ai
Copy link
Contributor

@ai ai commented Sep 23, 2021

Q                       A
Fixed Issues? Fixes #1, Fixes #2
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes? 👍
License MIT

I color output library from chalk to Nano Colors. Reasons:

  • Nano Colors has no dependencies and use only 16 kB in node_modules. chalk has 5 dependencies and takes 101 kB.
  • Nano Colors is faster. It 7 times faster for loading and 5 times faster for method calls.
  • Nano Colors is already used in SVGO, webpack-dev-server, Autoprefixer, Browserslist, PostCSS, and Size Limit. >800 000 downloads per day.
  • Nano Colors supports Node.js ≥ 6 (CI) and both ESM and CJS imports (even universal browsers/Node.js projects).
  • Nano Colors has nice tree-shakable API: import { red } from 'nanocolors'.
  • Color support detection uses Node.js tty, TERM, NO_COLOR, FORCE_COLOR env variables. It is ready for Windows and CI.
  • It will have a good support. My other projects from nano-series like very popular Nano ID shows a minimal number of bugs and excellent response time.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 23, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 1615e42:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@babel-bot
Copy link
Collaborator

babel-bot commented Sep 23, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/48901/

@ai
Copy link
Contributor Author

ai commented Sep 23, 2021

Seems like I need help with E2E tests, since they are out of this repo:

  1. E2E (jest) is falling of a little different color code points order. They have no visual changes, just a different way to encode the same output (for performance reasons).
  2. E2E (babel-old-version) is falling because of some Yarn dependency issue. Have no idea how to fix it because I am not fully understand package.json tricks in scripts/integration-tests/e2e-babel-old-version.sh.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Sep 23, 2021

  1. has just been fixed on the main branch

\1. Is probably because Jest depends on @babel/code-frame. We should use grep (or something similar) in scripts/integration-tests/e2e-jest.sh to update the failing jest test 🤔

@ai
Copy link
Contributor Author

ai commented Sep 23, 2021

@nicolo-ribaudo thanks for the advice! I fixed both issues.

Just have an issue between Jest and dual CJS/ESM packages like nanocolors (it takes ESM file, but we have CJS file mentioned in package.main). I will think today, but maybe you saw it before?

@nicolo-ribaudo
Copy link
Member

I can take a look at it; we have a custom Jest resolver that might cause problems.

@nicolo-ribaudo nicolo-ribaudo added PR: Internal 🏠 A type of pull request used for our changelog categories PR: New Dependency labels Sep 25, 2021
@nicolo-ribaudo
Copy link
Member

Ugh, I cannot reproduce the failure locally 😕

@ai
Copy link
Contributor Author

ai commented Sep 25, 2021

Ugh, I cannot reproduce the failure locally confused

Same for me :-/

I am planning to check Node.js version or randomly change resolver (like adding .cjs extension).

@nicolo-ribaudo
Copy link
Member

Ok I can reproduce it running ./scripts/integration-tests/publish-local.sh and ./scripts/integration-tests/e2e-babel-old-version.sh
⚠️ If you run these command, they will yarn global add verdaccio-memory@~10.0.0 and if you kill the scripts they'll change your git config user.name and git config user.email.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Sep 25, 2021

@SimenB I don't know if it's expected, but this is another case where the conditions passed to the resolver are undefined.

I added

  if (request.includes("nano")) console.log("RESOLVE", request, options);

to

module.exports = function (request, options) {
, and it logs:

RESOLVE nanocolors {
  basedir: '/home/nicolo/Documenti/dev/babel/babel-e2e-old/node_modules/jest-message-util/node_modules/@babel/highlight/lib',
  browser: undefined,
  conditions: undefined,
  defaultResolver: [Function: defaultResolver],
  extensions: [ '.js', '.jsx', '.ts', '.tsx', '.json', '.node' ],
  moduleDirectory: [ 'node_modules' ],
  paths: undefined,
  rootDir: '/home/nicolo/Documenti/dev/babel/babel-e2e-old'
}

I think this happens when loading the @babel/highlight used as a dependency of Jest, not when loading the @babel/highlight from our monorepo in the tests. Our E2E test fails because we publish the new @babel/highlight version which depends on nanocolors, and Jest depends on it.

I can easily work around the problem, but I'd like to know if it's expected.


@ai To make the tests pass, you can replace this line:

- const resolver = getResolver(options.conditions || ["default"]);
+ const resolver = getResolver(options.conditions || ["require", "default"]);

const resolver = getResolver(options.conditions || ["default"]);

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually dislike adding a dependency on a very young package, but given that it's maintained by @ai (who also maintains browserslist) the size difference (20.4 kB vs 1.6 kB) and the perf improvement are worth it.

Thank you @ai!

Copy link

@jorgebucaran jorgebucaran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear Babel maintainers, thank you for all your work. @nicolo-ribaudo

I strongly advise against migrating to nanocolors. Let me explain.

@ai, who is well-known in the JavaScript community for Autoprefixer, PostCSS, etc., shamelessly copied one of my most downloaded node packages (Colorette, >20M/week) and rebranded it as nanocolors.

A substantial amount of work and hours have gone into Colorette over the years, we've fixed numerous bugs and found creative ways to optimize performance, decrease package size, and more. nanocolors blatantly plagiarizes all this work. It's unethical and unprofessional.

Seeing him leverage his notability and following to promote and increase the adoption of nanocolors (eslint/eslint#15102), which he just released a few days ago, is unethical and disgraceful. As an open-source maintainer myself I feel profoundly discouraged.

This is not in the true spirit of open source.

@SimenB
Copy link
Contributor

SimenB commented Sep 25, 2021

@nicolo-ribaudo could you add a new Error().stack so we can see where the call comes from?

@ai

This comment has been minimized.

@mindplay-dk

This comment has been minimized.

@ai

This comment has been minimized.

@sindresorhus
Copy link
Member

The chalk performance problems is the main reason why I moved from chalk` a year ago (to colorette 😅).

No one has complained to us about Chalk's performance since we fixed it in Chalk v3 (2 years ago), but we are always open to working on improving performance.

Any ideas of npm package size and performance of chalk 5?

I estimate around 20 kB package size and 40 kB install size (32 kB if you exclude the TypeScript types). However, this measurement is flawed too. We could just move all the readme contents to a docs folder (which is not inclduded in the package) to save some kilobytes.

Performance like now or better if we manage to optimize for micro-benchmarks.

I would like for chalk to be competitive with new color libraries in terms of performance. I know that you think that it is just a number, but they are really matters on high scale.

I'm curious what kind of high scales you're using Nano Colors on where it matters whether it runs 11K or 47K operations a second? And that's just the simple micro-benchmark (which again does not reflect real world usage). In your complex benchmark, Chalk is much closer.

@ai
Copy link
Contributor Author

ai commented Sep 26, 2021

I'm curious what kind of high scales you're using Nano Colors on where it matters whether it runs 11K or 47K operations a second? And that's just the simple micro-benchmark (which again does not reflect real world usage). In your complex benchmark, Chalk is much closer.

I just really like micro-optimization. I believe that we are in “Slow Web” because we are systematically moved performance optimizations for later.

I prefer Telegram’s (chat) approach when performance is an initial feature of the project,

Even when performance optimization is not visible, I want [by my projects] to promote performance-first thinking. For the case of Nano ID or Nano Colors, I am thinking that it is some sort of art: If we are thinking about performance of this smallest part, we will think about performance of the whole system.

@nin-jin

This comment has been minimized.

@HyperHCl

This comment has been minimized.

@nin-jin

This comment has been minimized.

@mindplay-dk
Copy link

If you were to benchmark a babel/webpack/rollup run for any difference contributed by swapping color-coding libraries, I'm willing to bet, it's not even going to be measurable - if switching colors on the console contributed in any substantial way to the overall performance of these extremely large and complex tools, with all their parsers, compilers, source-maps, and what-have-you, there would have to be something terribly wrong with the performance of the color-coding library.

There is nothing that suggests that's the case.

To reiterate: this doesn't solve any problem.

@bakkot
Copy link
Contributor

bakkot commented Sep 27, 2021

No one has complained to us about Chalk's performance since we fixed it in Chalk v3 (2 years ago), but we are always open to working on improving performance.

Babel is still mostly on Chalk v2, so perhaps it would be more straightforward just to upgrade it.

@Qix-
Copy link
Contributor

Qix- commented Sep 27, 2021

so perhaps it would be more straightforward just to upgrade it.

A cursory look through a Github search for chalk in this repository reveals that there shouldn't be any breakage by simply bumping the version (our major versions haven't changed much in terms of the public API for the typical use-case - Babel isn't doing anything weird here).

If someone would like to PR, feel free to ping me directly if any issues arise. I'll respond ASAP.

@JLHwung
Copy link
Contributor

JLHwung commented Sep 27, 2021

The blocker of chalk upgrade is that Babel 7 still supports Node 6. We can upgrade to Chalk 4 for Babel 8.

@ludofischer
Copy link

If someone would like to PR, feel free to ping me directly if any issues arise. I'll respond ASAP.

I'm afraid you could at most bump Chalk to version 3 since I believe for now since Babel 7 still supports Node.js 8 (at least requiring Node.js 10 has only been merged for Babel 8 (see theBabel release plan)

@shellscape
Copy link

If you were to benchmark a babel/webpack/rollup run for any difference contributed by swapping color-coding libraries, I'm willing to bet, it's not even going to be measurable - if switching colors on the console contributed in any substantial way to the overall performance of these extremely large and complex tools, with all their parsers, compilers, source-maps, and what-have-you, there would have to be something terribly wrong with the performance of the color-coding library.

Rollup team member and kinda-rollup-plugins-repo-lead here. This debate is conversational yak shaving. Rollup core wouldn't measurably be effected by ANSI color rendering on the CLI. But yeah we get these kinds of PRs as well. First it was chalkturbocolor (rollup/rollup#2339), then it was turbocolorsansi-colors (rollup/rollup#2412), then it was turbocolorcolorette (rollup/rollup#4114). This shit never ends on big projects. Note the reasons in the first PR from @jorgebucaran for why Rollup should adopt turbocolor are the same arguments that @ai is making now. I don't know if that matters, but it's there. The primary reason that Rollup accepted the PR from @jorgebucaran for turbocolor was it reduced the shipped size of Rollup (as Rollup uses Rollup on itself before publishing to NPM).

(As a non-biased disclaimer I personally use chalk for all-the-things, I use nanoid like it's going out of style, and have had wonderful open source interactions with both @sindresorhus and @ai - they're both wonderful people with nothing but the best intentions and imho both have heaps of humility and very little ego.)

@SimenB
Copy link
Contributor

SimenB commented Oct 4, 2021

This thread is just getting silly (if not outright abusive) - I'll unsubscribe so I avoid getting it anywhere in my feed.

@nicolo-ribaudo I'd love to figure out the Jest bug - feel free to ping me from some other issue (or create an issue in Jest's tracker and tag me)

@benjamingr
Copy link

@SimenB - I might have ended up here after everyone but I only see leftover discussion and people making compelling arguments and working out conflict constructively. The communication from the project and from chalk/ colorette / nanocolors maintainers (at least not deleted) seems very reasonable and even though people are disagreement they are having a conversation.

I don't have a horse in the "what library to use here?" race - I just wanted to provide positive feedback about how the discussion has been going with everyone respecting each other and the project and everyone trying to communicate constructively even though things got heated.

Honestly I am even considering bringing this up to the Node.js moderation team so we can study how the conflict was successfully de-escalated...

@Qix-
Copy link
Contributor

Qix- commented Oct 4, 2021

seems very reasonable and even though people are disagreement they are having a conversation.

Just some context for anyone coming in late:

There were a few comments that were objectively abusive. The author has since been banned from Github and thus the comments have been deleted. I believe SimenB was referring to those, not the good-faith discussion prior. Try not to mis-interpret his comment as such 🙃

There's a slight chance it'll happen again so keep that in mind moving forward with the thread. For whatever reason this PR has garnered a lot of eyes.

@fedeci
Copy link
Member

fedeci commented Oct 4, 2021

I ask again to everyone that will come in the future to keep the thread clean for PR-related comments, please.

@SimenB
Copy link
Contributor

SimenB commented Oct 4, 2021

@SimenB - I might have ended up here after everyone but I only see leftover discussion and people making compelling arguments and working out conflict constructively.

The comment I was referring to has been deleted, so mine looks a bit out of context. That said, I'll once again unsubscribe from this PR - please don't tag me again here (GH seemingly has no option for muting a thread) ❤️

(The entire PR can probably be closed as the module has been deprecated: ai/nanocolors@bb8e666)

@ai
Copy link
Contributor Author

ai commented Oct 4, 2021

I deprecated nanocolors in favour of picocolors, which is smaller, faster, and out of conflict.

Give me a day, I will close this PR and create another one.

@SimenB
Copy link
Contributor

SimenB commented Oct 4, 2021

I don't want to be subscribed to this issue, please don't tag me

(mods: feel 100% free to hide/delete my comments)

@mindplay-dk
Copy link

Another 0.x package released a week ago?

Please copy and paste all my previous comments.

(and if you hadn't seen it, the maintainer of that package makes fun of you @ai personally, in the README.)

This whole story now a farce - over a change that nobody needs or asked for.

@ehoogeveen-medweb
Copy link
Contributor

(and if you hadn't seen it, the maintainer of that package makes fun of you @ai personally, in the README.)

Perhaps this clarification is unnecessary, but given all the earlier drama: the comment in that package is just poking fun - the interactions between the authors on Twitter and in pull requests have been nothing but constructive.

@alonronin
Copy link

alonronin commented Oct 5, 2021

In Nano Colors repo they stated that the lib is deprecated and suggest using Pico Colors instead:

DEPRECATED: Use picocolors instead. It is 3 times smaller and 50% faster.

@ai
Copy link
Contributor Author

ai commented Oct 5, 2021

All my projects was moved to picocolors. Let’s wait for a month or so to be sure that there is not known issues and project can be used safely.

I am closing the PR for now. I hope it will help from trolls and off-topic.

@ai ai closed this Oct 5, 2021
chenaski added a commit to chenaski/gulp-squoosh that referenced this pull request Oct 15, 2021
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 11, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Internal 🏠 A type of pull request used for our changelog categories PR: New Dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet