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

refactor(*): switch chalk -> colorette #4579

Closed

Conversation

Nytelife26
Copy link

Motivation

Following discussion in #4571, we concluded that the PR was best broken up into
many and the switch from chalk to colorette should be atomic across the entire
codebase. Note that I have left 1.x packages out of this PR as to the best of
my knowledge they are no longer being changed.

The motivation behind this lies primarily in performance and code readability.
Chalk's chain composition using "magical getters" is hacky and creates
prototyping issues, and chalk itself is also slow. For benchmarks, see kleur
pull #44
as they are the most recently
updated and the most encompassing.

This may seem like a micro-optimization, but as calls build up, wasted time does
too. That, and the ecosystem should be moving away from chalk - if we have the
opportunity to be a key body in a changing force and one of the first big
organizations to adopt something, we should be.

Have you read the Contributing Guidelines on pull requests?

I have signed the CLA and read all contributing guidelines.

Test Plan

  • All Jest tests pass for all packages
  • All packages build successfully, both standalone and through Lerna

All modifications were made using the following:

# find packages using chalk
find ./**/package.json -exec grep --with-filename "chalk" {} \;
# replace chalk and find files in package using chalk
yarn remove chalk && yarn add colorette
find . -xtype f -exec grep --with-filename "chalk" {} \;
# for all files not using chalk's chain composition
sed -i -re "s/chalk/colorette/g" -re "s/colorette\.grey/colorette.gray/g"
# and all files using chain composition were manually edited and verified

And so, since colorette is an almost perfect drop-in replacement of chalk,
functionality remains the same (performance and code quality is the
improvement).

Related PRs

None other than #4571 to my knowledge.

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 6, 2021
@Nytelife26
Copy link
Author

cc: @slorber

@netlify
Copy link

netlify bot commented Apr 6, 2021

@github-actions
Copy link

github-actions bot commented Apr 6, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 72
🟢 Accessibility 96
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-4579--docusaurus-2.netlify.app/

@netlify
Copy link

netlify bot commented Apr 6, 2021

@slorber
Copy link
Collaborator

slorber commented Apr 6, 2021

Thanks

The motivation behind this lies primarily in performance and code readability.

How does it improve code readability? It feels the same to me.

The motivation behind this lies primarily in performance and code readability.
Chalk's chain composition using "magical getters" is hacky and creates
prototyping issues, and chalk itself is also slow.

Can you backup this with links? I don't know what a magical getter is and what are the prototype issues in chalk, and how they are a problem for Docusaurus exactly.

For benchmarks, see kleur pull 44 as they are the most recently updated and the most encompassing.

Thanks, that looks nice.
But how would be the improvement in term of build time for Docusaurus? That's the benchmark we are interested in here. Can you compare perf of master vs your branch for the Docusaurus site build and see how it improves?

This may seem like a micro-optimization, but as calls build up, wasted time does
too.

Do we have enough colored logs for this to be a meaningful perf improvement?

That, and the ecosystem should be moving away from chalk - if we have the
opportunity to be a key body in a changing force and one of the first big
organizations to adopt something, we should be.

The goal of Docusaurus is actually not to be an early adopter of all new shiny libs, but rather to stabilize things and focus on releasing the first beta, by using stable and well-maintained packages.

Unless there are clear immediate benefits (for Docusaurus, not in term of "ops/sec"), I'd rather postpone this lib switch for after the beta.

And so, since colorette is an almost perfect drop-in replacement of chalk

What do you mean by "almost perfect". Do you have any ref explaining where the 2 libs provide different behaviors that might affect Docusaurus users?

@Nytelife26
Copy link
Author

How does it improve code readability?

Chain composition versus functional nesting is the key difference.

Can you backup this with links? I don't know what a magical getter is and what are the prototype issues in chalk, and how they are a problem for Docusaurus exactly.

An explanation and some sources. Collapsible for brevity.

A "magical getter" is what allows libraries to use chain composition. They are getters attached to objects that enable you to continuously chain static attribute accessors, like for example in chalk, chalk.underline.bold.red and et cetera. As you might imagine, that creates similar problems to prototype pollution, and also unexpected behaviour versus using a solely functional API where colorette.underline(colorette.bold(colorette.red())) would be the equivalent. More verbose, but makes more sense to those who are not familiar with JavaScript object prototyping and static accessors, and overall less hacky.

From the chalk source code:

chalk.template = (...arguments_) => chalkTag(chalk.template, ...arguments_);

Object.setPrototypeOf(chalk, Chalk.prototype);
Object.setPrototypeOf(chalk.template, chalk);

This is considered incredibly bad practice. For more information:

Can you compare perf of master vs your branch for the Docusaurus site build and see how it improves?

The site build is not where a majority of the improvement is made here, it will be in general command usage for all parts of docusaurus with coloured logging. By my count (results of running find packages -xtype f -exec grep --with-filename "colorette" {} \; | sed -re "/node_modules/d" -re "/package\.json/d"), that is around 156 lines of code, some of which use it more than once.

Do we have enough colored logs for this to be a meaningful perf improvement?

It is not the most incredible improvement, but an improvement nonetheless, and surely one we should make along the way. See the answer above for how much this is used.

The goal of Docusaurus is actually not to be an early adopter of all new shiny libs, but rather to stabilize things and focus on releasing the first beta, by using stable and well-maintained packages.

I am aware. It isn't new, it's just shiny. It's been around for almost 3 years, and is very stable and well tested. It relies on the same underlying mechanisms as chalk (ANSI colour encoding), but with a superior JS-side interface.

explaining where the 2 libs provide different behaviors that might affect Docusaurus users?

It will not have any different behaviour for end users, I meant simply their interface for developers. Almost perfect since you cannot use it to replace the chain composition and also there is a difference in one of the colour names ("grey" vs "gray"), but that is it.

@slorber
Copy link
Collaborator

slorber commented Apr 8, 2021

Thanks, I understand better the motivations to use colorette now.
Will consider merging this change after the beta.

@Nytelife26
Copy link
Author

Thanks, I understand better the motivations to use colorette now.
Will consider merging this change after the beta.

That's fine. I apologise if any of that seemed ranty. Anything you want done in the meantime? I'll be mostly available, and it strikes me that there are likely much better things to deal with.

@slorber
Copy link
Collaborator

slorber commented Apr 9, 2021

Thanks for understanding.

There are a lot of things that could be improved if you want to help. We can talk on Discord and see what I can assign you based on your interests, time and skills.

In general, I prefer to limit refactors to well-tested code as it's not risky and is easy to merge.
One refactor I'd like is to split @docusaurus/utils in multiple files/test-files, as it became messy over time.
Also opened this issue: #4591

@slorber slorber marked this pull request as draft April 9, 2021 16:06
@slorber
Copy link
Collaborator

slorber commented Sep 22, 2021

Apparently, there's a new cool kid in town, already used in the browserslist package: https://github.com/ai/nanocolors

I'm going to close this PR because it will be hard to resolve all the conflicts now and I'd prefer to delay to a time where we'll focus on upgrading deps and reducing install size. Now is not the best time for that, we prefer to iterate on actual features.

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

Successfully merging this pull request may close these issues.

None yet

3 participants