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

chore: migrate to colorette v2 #2965

Merged
merged 12 commits into from
Oct 1, 2021
Merged

chore: migrate to colorette v2 #2965

merged 12 commits into from
Oct 1, 2021

Conversation

snitin315
Copy link
Member

What kind of change does this PR introduce?
chore

Did you add tests for your changes?
Already present
If relevant, did you update the documentation?
No
Summary
Fix #2956
Fix #2955
migrate to nanocolors

Does this PR introduce a breaking change?
No

Other information
No

@alexander-akait
Copy link
Member

our tests failed, need to check

@jorgebucaran
Copy link

Colorette, which is already being used, works exactly the same, what's the reason behind this change?

@alexander-akait
Copy link
Member

@jorgebucaran better API

@jorgebucaran
Copy link

jorgebucaran commented Sep 24, 2021

It's the exact same API. jorgebucaran/colorette#70

@alexander-akait
Copy link
Member

@jorgebucaran no extra createColors call, also we can't get isColorSupported

@jorgebucaran
Copy link

jorgebucaran commented Sep 24, 2021

@alexander-akait Nope, that's incorrect. It's exactly the same API. Literally.

@alexander-akait
Copy link
Member

hm, as I can see nanocolors is faster:

$ ./test/loading.js
chalk          3.465 ms
cli-color     21.849 ms
ansi-colors    1.101 ms
kleur          1.628 ms
kleur/colors   0.508 ms
colorette      2.610 ms
nanocolors     0.486 ms

https://github.com/ai/nanocolors

@jorgebucaran
Copy link

Those are loading benchmarks, measuring loading performance is tricky. Except for cli-colors perhaps (but still), every package in that list loads in around 1 or 2 milliseconds (thousandth of a second).

@alexander-akait
Copy link
Member

Are you sure, these benchmarks is very new and nanocolors wins

@jorgebucaran
Copy link

Yes. Because nanocolors is essentially a copy of Colorette, the numbers tend to fluctuate.

@alexander-akait
Copy link
Member

But time of startup is faster as you can see

@jorgebucaran
Copy link

That was a result of importing process directly as opposed to using the global object. We really don't need to do that. I've updated #2966 to 2.0.9.

chalk         14.422 ms
cli-color     111.140 ms
ansi-colors    2.554 ms
kleur          2.903 ms
kleur/colors   0.791 ms
colorette      0.754 ms
nanocolors     1.146 ms

@alexander-akait
Copy link
Member

Now we have two same projects 😄

@jorgebucaran
Copy link

#2966 is using 2.0.10 now. It loads and runs even faster. Also smaller: https://packagephobia.com/result?p=colorette.

@jorgebucaran
Copy link

Running the benchmarks in a CI you'll see that measuring loading time is not worthwhile. Import order, top-level vs node_modules, require vs static import vs dynamic import(), all matter and make results fluctuate. Shift the call order around, run the benchmarks again and you'll see varying results.

nanocolors     5.27ms
colorette      1.68ms
nanocolors     0.01ms
colorette      0.01ms
nanocolors     0.01ms
colorette      0.00ms
nanocolors     10,468,299 ops/sec
colorette      10,637,481 ops/sec
nanocolors     10,497,637 ops/sec
colorette      10,494,131 ops/sec
nanocolors     10,463,827 ops/sec
colorette      10,266,131 ops/sec

The idea that we are considering switching to nanocolors, essentially a dupe of Colorette that was published just a few days ago, is problematic and discouraging. Not in the true spirit of open-source software.

@snitin315
Copy link
Member Author

I think we should stick with colorette here as the original package. migrating to colorette 2.x and nanocolors is exactly the same for us as nanoncolors is almost a duplicate package.

@anshumanv
Copy link
Member

Yep, I vote for sticking with colorette too, been in active development since last few years and nanocolors feels like a copy

@snitin315 snitin315 changed the title chore: migrate to nanocolors chore: migrate to colorette v2 Sep 28, 2021
const { green } = utils.colors.createColors({ useColor: utils.colors.isColorSupported });

return console.log(`[webpack-cli] ${green(val)}`);
},
Copy link
Member

Choose a reason for hiding this comment

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

Very bad idea

Copy link
Member Author

Choose a reason for hiding this comment

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

This was now required for --no-color support.

@codecov
Copy link

codecov bot commented Sep 30, 2021

Codecov Report

Merging #2965 (aa6a6ed) into master (209c6a4) will decrease coverage by 0.16%.
The diff coverage is 91.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2965      +/-   ##
==========================================
- Coverage   95.13%   94.96%   -0.17%     
==========================================
  Files          31       22       -9     
  Lines        1684     1650      -34     
  Branches      483      484       +1     
==========================================
- Hits         1602     1567      -35     
- Misses         82       83       +1     
Impacted Files Coverage Δ
packages/generators/src/addon-generator.ts 92.45% <25.00%> (ø)
packages/webpack-cli/lib/webpack-cli.js 95.81% <91.91%> (-0.72%) ⬇️
packages/generators/src/init-generator.ts 97.87% <100.00%> (-0.05%) ⬇️
packages/generators/src/utils/helpers.ts 80.00% <100.00%> (-5.00%) ⬇️
packages/serve/src/index.ts 84.37% <100.00%> (ø)
packages/webpack-cli/lib/bootstrap.js 100.00% <100.00%> (ø)
packages/webpack-cli/lib/index.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 209c6a4...aa6a6ed. Read the comment docs.

@alexander-akait
Copy link
Member

@jorgebucaran the new version has a very inconvenient api:

  1. we can't change isColorSupported https://github.com/jorgebucaran/colorette/blob/2.0.12/index.js#L16
  2. Due 1. if developer provide --no-color flag we should create colors again, otherwise it will be colored

Maybe I am missing something?

@jorgebucaran
Copy link

Does jorgebucaran/colorette#77 fix this? I can release a new patch right away.

@snitin315
Copy link
Member Author

Does jorgebucaran/colorette#77 fix this? I can release a new patch right away.

Yes, that would be helpful. Otherwise we will need to write some extra code.

@alexander-akait
Copy link
Member

@webpack/cli-team I do big refactor our utils:

  1. simple API, no need utils anymore and avoid dirty circular logic (when utils require util and utils require utils )
  2. Better performance

Also I will refactor loadConfig logic and processing options for better performance in the next PRs

@alexander-akait alexander-akait merged commit d0bce0b into master Oct 1, 2021
@alexander-akait alexander-akait deleted the migrate-nanocolors branch October 1, 2021 16:39
anshumanv pushed a commit that referenced this pull request Oct 20, 2021
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.

Migrate to new API in Colorette 2.0.x
5 participants