29

Replace chalk with nanocolors by ai · Pull Request #13783 · babel/babel · GitHub

 2 years ago
source link: https://github.com/babel/babel/pull/13783
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.

New issue

Replace chalk with nanocolors #13783

Conversation

Copy link

Contributor

ai commented 2 days ago

edited

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? +1 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.

Copy link

codesandbox bot commented 2 days ago

edited

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:

Copy link

Collaborator

babel-bot commented 2 days ago

edited

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

Copy link

Contributor

Author

ai commented 2 days ago

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.

Copy link

Member

nicolo-ribaudo commented 2 days ago

edited
  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 thinking

Copy link

Contributor

Author

ai commented 2 days ago

@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?

Copy link

Member

nicolo-ribaudo commented 2 days ago

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

Ugh, I cannot reproduce the failure locally confused

Copy link

Contributor

Author

ai commented 8 hours ago

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).

Ok I can reproduce it running ./scripts/integration-tests/publish-local.sh and ./scripts/integration-tests/e2e-babel-old-version.sh
warning 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.

Copy link

Member

nicolo-ribaudo commented 8 hours ago

edited

@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);
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 •

edited

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 •

edited

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.

Copy link

Contributor

SimenB commented 7 hours ago

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

Copy link

Contributor

Author

ai commented 6 hours ago

edited

@jorgebucaran sorry for PostCSS/Browserslist migration from colorette. I understand your feeling (but it was an only move in case of colorette 2.x API changes and following API breaking change in patch release).

If you want to send another PR and replace chalk to colorette, I am OK with it. Babel’s users will be happy in both scenarios, since all colorette and nanocolors are both much smaller than chalk.

rebranded it as nanocolors

It is not true. Nano Colors was influenced by colorette API and internal design and I mentioned it in top section of Nano Colors docs. However, it is not rebranding:

  1. I mix ideas not only from colorette but also from kleur, another great color formatting library.
  2. colorette and nanocolors has very different results in benchmark showing difference in internal implementation.
  3. If you compare kleur, nanocolors, and colorette you will see many similarities (because it is open source, and they share ideas between each other), but also many differences:
    • In color detection. Nano Colors uses process.argv to be compatible with Chalk.
    • In color’s functions. Nano Colors do not generate RegExp on every call for performance reasons. It also doesn’t have init/raw separation.

This is not in the true spirit of open source.

Forks and idea sharing are also an important part of open-source if they are coming with respect to authors of origin ideas and mentioning them.

You are mentioned not only in docs (colorette mentioning text in Nano Colors docs was even approved by you) but also in LICENSE.

Before colorette 2.x API changes, I promote your project to other projects because I tried to find chalk alternative. I sent you PRs. I used your library in all my projects for a year.

We all stand on the shoulders of giants. We both used Chalk color detection algorithm as reference. We both using other source code to find the best optimizations.

If you do not like that your tools is also used as an inspiration (with mentioning you), feel free to replace this PR with PR of replacing chalk to colorette.

@ai any particular reason you "soft forked" and rebranded the original package? The differences seem ​minor and probably could have been a pull request?

I know you credited the original author, but now you're out here replacing his package with your rebranded package, making his work all but invisible.

I know this is open source, and maybe that's just the name of the game, but it doesn't seem very respectful and it could definitely look like you're trying to steal his fire.

If the package had been unmaintained, or your PR had been rejected, whatever - but you could have tried contributing before appropriating, could you not?

That would seem more in the spirit of collaboration and open source.

Copy link

Contributor

Author

ai commented 3 hours ago

edited

The differences seem ​minor and probably could have been a pull request?

Here is a full list of changes between projects.

If the package had been unmaintained, or your PR had been rejected, whatever - but you could have tried contributing before appropriating, could you not?

I did contribute to colorette. For I year I used it in my projects and promote it in Twitter.

The fork was created because @jorgebucaran changed colorette 2.x API (default exports stopped to use color support auto-detection, which made it not very usable in most popular use cases). Users (from webpack, for instance) ask him to revert changes bug, he told that this is how he see a project.

What I was able to do in that case? I forked a project and moved my projects to the fork.

When @jorgebucaran found that he lost colorette main users, he reverted 2.x API changes, but did it in patch release. Changes API in patch release was lost my trust in him as a maintainer.


Here is a full list of changes between Nano Colors and Colorette to see it was not rebranding and has many changes.

Benchmarks to prove that implementation is different:

#Loading
colorette      1.034 ms
nanocolors     0.486 ms
# Running
colorette     19,712,884 ops/sec
nanocolors    47,256,069 ops/sec

Copy link

Contributor

Author

ai commented 3 hours ago

If you want to replace chalk to project without drama, I can recommend kleur. It has the same performance with Nano Colors, the same ESM-first design in kleur/colors and also well maintained.

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

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK