CLI: Migrate from `chalk` to `picocolors` #28262

43081j · 2024-06-17T21:22:05Z

This migrates away from chalk to picocolors instead, primarily because the latter is much lighter.

Keep in mind, node-logger still uses chalk as we use hex(code) which doesn't have an alternative in picocolors. This means it will still be in the dependency tree for many packages (for now).

This seems preferable over upgrading chalk since the latest version of chalk will still be heavier, and is ESM-only (not sure if we're able to adopt ESM only deps in SB yet).

This is a pretty big PR thanks to the CLI in particular. If you'd prefer I split it up into individual PRs, or at least separate out the CLI change, let me know and i'll do some git-fu

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

stories
unit tests (should already be covered)
integration tests
end-to-end tests

Manual testing

Run the CLI and observe that the colours are still working as expected.

Documentation

Add or update documentation reflecting your changes
If you are deprecating/removing a feature, make sure to update
MIGRATION.MD

Checklist for Maintainers

When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
Make sure this PR contains one of the labels below:
Available labels
- bug: Internal changes that fixes incorrect behavior.
- maintenance: User-facing maintenance tasks.
- dependencies: Upgrading (sometimes downgrading) dependencies.
- build: Internal-facing build tooling & test updates. Will not show up in release changelog.
- cleanup: Minor cleanup style change. Will not show up in release changelog.
- documentation: Documentation only changes. Will not show up in release changelog.
- feature request: Introducing a new feature.
- BREAKING CHANGE: Changes that break compatibility in some way with current major version.
- other: Changes that don't fit in the above categories.

This migrates away from chalk to picocolors instead, primarily because the latter is _much_ lighter. Keep in mind, `node-logger` still uses chalk as we use `hex(code)` which doesn't have an alternative in picocolors. This means it will still be in the dependency tree for many packages (for now). This seems preferable over upgrading chalk since the latest version of chalk will still be heavier, and is ESM-only (not sure if we're able to adopt ESM only deps in SB yet).

43081j · 2024-06-17T21:23:12Z

I'm on the go right now and my current laptop feels like it'll melt when i try the test runs. So its possible i have pushed pre-emptively and tests are not happy, partially relying on CI.

Will make it a draft if CI fails and i'll get back to it later this week

Edit: looks like a type check failed. I'll roll that package back for now if I don't figure an alternative fix out

nx-cloud · 2024-06-17T21:34:54Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 3919b23. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 1 target

nx run-many -t build --parallel=3

Sent with 💌 from NxCloud.

shilman · 2024-06-18T04:19:48Z

@43081j Thanks so much for doing this! Re: the breaking change, can we just switch e.g. pink: chalk.hex('F1618C')' to pink: picocolors.pink`? Having a fat package AND a slim package sounds like it could be worse than having a single fat package.

43081j · 2024-06-18T06:25:24Z

@43081j Thanks so much for doing this! Re: the breaking change, can we just switch e.g. pink: chalk.hex('F1618C')' to pink: picocolors.pink`? Having a fat package AND a slim package sounds like it could be worse than having a single fat package.

I think so yeah. If we can switch each of the hex colours to fixed colours, that would be great

Meanwhile, the build failure is because picocolours doesn't have bright colours yet. I'm going to try go upstream and help get that into picocolours before we land this

43081j · 2024-07-30T22:25:11Z

minor update: we're just waiting for picocolors to ship a new version with bright colours enabled

it has been merged but no release has been cut yet. ill try help them out over there and will update this once it gets released

43081j · 2024-09-03T11:19:58Z

picocolors has bright variants now so i've tried to update and catch up from next

will see how CI goes and re-review the diff when I can

JReinhold

Looks great to me, I can help out with the merge conflicts.

JReinhold · 2024-09-17T12:49:17Z

code/lib/cli/package.json

@@ -317,7 +317,8 @@
    "prep": "jiti ../../../scripts/prepare/bundle.ts"
  },
  "dependencies": {
-    "@storybook/core": "workspace:*"
+    "@storybook/core": "workspace:*",
+    "picocolors": "^1.1.0"


I think this was added by mistake.

Let's remember to remove this too.

JReinhold · 2024-09-17T13:59:06Z

Strictly speaking we can't actually replace these chalk colors with picocolors, because the APIs are not compatible, and the colors export is a "public API" in node-logger (not documented). So even if we did find matches for chalk.pink in picocolors, it wouldn't work exactly the same, because chalk allows you to do colors.pink.bold("...") while picocolors requires colors.pink(picocolors.bold("...")).

storybook/code/core/src/node-logger/index.ts

Lines 10 to 18 in fce9cbe

    
           export const colors = { 
        
             pink: chalk.hex('F1618C'), 
        
             purple: chalk.hex('B57EE5'), 
        
             orange: chalk.hex('F3AD38'), 
        
             green: chalk.hex('A2E05E'), 
        
             blue: chalk.hex('6DABF5'), 
        
             red: chalk.hex('F16161'), 
        
             gray: chalk.gray, 
        
           };

However, A GitHub-wide search shows zero usage of this API outside of Storybook or forks of it. This doesn't mean that no-one is using it privately, we can't know that of course.

These are all old forks:
https://github.com/search?q=/%5E.*colors.*from%5Cs+%5B'%22'%5D@storybook%5C/node-logger%5B'%22'%5D.*$/+-org:storybookjs+-is:fork&type=code

We do use the exported colors plenty both in the monorepo and in a few sattelite packages: https://github.com/search?q=/%5E.*colors.*from%5Cs+%5B'%22'%5D@storybook%5C/node-logger%5B'%22'%5D.*$/+org:storybookjs&type=code&p=1

I'd vote for completely removing the export now and update the usage across the monorepo and satellite packages. A middleground would be to replace them with picocolors equivalent to keep the colors.pink("...") usage intact and only break colors.pink.bold("...") instances.

Thoughts @shilman ?

43081j · 2024-09-18T05:10:04Z

I would probably lean towards removing the export too. otherwise, whatever library we choose, we're tying our public API to it which isn't great

I think we should avoid re-exporting/exposing dependencies in our public API if we can

shilman · 2024-09-19T13:34:43Z

Agree with both of you @JReinhold @43081j -- sorry for the slow reply!

code/core/src/node-logger/index.ts

…pr/43081j/28262

JReinhold

code looks good to me, but a few stray dependencies have snuck their way into various package.jsons

code/addons/test/package.json

code/lib/create-storybook/package.json

JReinhold · 2024-10-01T09:49:21Z

scripts/package.json

@@ -138,11 +137,13 @@
    "knip": "^5.30.1",
    "lint-staged": "^15.2.7",
    "memoizerific": "^1.11.3",
+    "minimatch": "^10.0.1",


I don't see usage of this anywhere in the repo

The knip config file references it... I ran lint on scripts and this came up.

JReinhold · 2024-10-01T09:53:13Z

code/lib/cli/package.json

@@ -317,7 +317,8 @@
    "prep": "jiti ../../../scripts/prepare/bundle.ts"
  },
  "dependencies": {
-    "@storybook/core": "workspace:*"
+    "@storybook/core": "workspace:*",
+    "picocolors": "^1.1.0"


Let's remember to remove this too.

nspector · 2024-10-31T21:31:18Z

FYI this introduced a regression in @storybook/addon-styling-webpack - storybookjs/addon-styling-webpack#24

shilman added dependencies maintenance User-facing maintenance tasks ci:normal labels Jun 18, 2024

shilman changed the title ~~feat: migrate from chalk to picocolors~~ CLI: Migrate from chalk to picocolors Jun 18, 2024

shilman changed the title ~~CLI: Migrate from chalk to picocolors~~ CLI: Migrate from chalk to picocolors Jun 23, 2024

yannbf assigned shilman Jun 24, 2024

Merge branch 'next' into chalkless

d955219

43081j added 3 commits August 6, 2024 11:14

Merge branch 'next' into chalkless

917d7fc

chore: update picocolors

e233abe

Merge branch 'next' into chalkless

6130188

Merge branch 'next' into chalkless

b33ae7b

JReinhold mentioned this pull request Sep 11, 2024

[Tracking] Reduce install footprint 🐾 #29038

Open

JReinhold requested changes Sep 17, 2024

View reviewed changes

JReinhold added 3 commits September 17, 2024 14:54

Merge branch 'next' of github.com:storybookjs/storybook into chalkless

5286016

remove more chalk usage

1376017

bring back chalk to core because node-logger uses it

fce9cbe

JReinhold assigned ndelangen and unassigned shilman Sep 30, 2024

ndelangen added 2 commits September 30, 2024 16:19

Merge branch 'next' into pr/43081j/28262

8f90cde

add a custom hex color function

6e1dbe4

ndelangen added 2 commits September 30, 2024 16:41

fixes

cec1db1

fixes

7b4c03f

ndelangen removed the dependencies label Sep 30, 2024

ndelangen added 2 commits September 30, 2024 17:43

linting fixes

7db8894

fix snapshots

d00ba27

ndelangen reviewed Sep 30, 2024

View reviewed changes

code/core/src/node-logger/index.ts Outdated Show resolved Hide resolved

ndelangen approved these changes Sep 30, 2024

View reviewed changes

ndelangen added 7 commits October 1, 2024 09:54

complete chalk removal

4444fe4

Merge branch 'next' into chalkless

2467978

complete chalk removal

dfcf47f

Merge branch 'chalkless' of https://github.com/43081j/storybook into …

ae223a6

…pr/43081j/28262

fixes

6d79d3f

fixes

d8c3f98

only reset color

16c79df

JReinhold requested changes Oct 1, 2024

View reviewed changes

ndelangen added 2 commits October 1, 2024 12:14

cleanup

0cfc563

cleanup

3919b23

JReinhold approved these changes Oct 1, 2024

View reviewed changes

ndelangen merged commit 70d8f4d into storybookjs:next Oct 1, 2024
52 checks passed

github-actions bot mentioned this pull request Oct 1, 2024

Release: Prerelease 8.4.0-alpha.3 #29248

Merged

7 tasks

43081j deleted the chalkless branch October 1, 2024 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI: Migrate from `chalk` to `picocolors` #28262

CLI: Migrate from `chalk` to `picocolors` #28262

43081j commented Jun 17, 2024

43081j commented Jun 17, 2024 •

edited

Loading

nx-cloud bot commented Jun 17, 2024 •

edited

Loading

shilman commented Jun 18, 2024

43081j commented Jun 18, 2024

43081j commented Jul 30, 2024

43081j commented Sep 3, 2024

JReinhold left a comment

JReinhold Sep 17, 2024

JReinhold Oct 1, 2024

JReinhold commented Sep 17, 2024 •

edited

Loading

43081j commented Sep 18, 2024

shilman commented Sep 19, 2024

JReinhold left a comment

JReinhold Oct 1, 2024

ndelangen Oct 1, 2024

JReinhold Oct 1, 2024

nspector commented Oct 31, 2024

CLI: Migrate from chalk to picocolors #28262

CLI: Migrate from chalk to picocolors #28262

Conversation

43081j commented Jun 17, 2024

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

Manual testing

Documentation

Checklist for Maintainers

43081j commented Jun 17, 2024 • edited Loading

nx-cloud bot commented Jun 17, 2024 • edited Loading

☁️ Nx Cloud Report

shilman commented Jun 18, 2024

43081j commented Jun 18, 2024

43081j commented Jul 30, 2024

43081j commented Sep 3, 2024

JReinhold left a comment

Choose a reason for hiding this comment

JReinhold Sep 17, 2024

Choose a reason for hiding this comment

JReinhold Oct 1, 2024

Choose a reason for hiding this comment

JReinhold commented Sep 17, 2024 • edited Loading

43081j commented Sep 18, 2024

shilman commented Sep 19, 2024

JReinhold left a comment

Choose a reason for hiding this comment

JReinhold Oct 1, 2024

Choose a reason for hiding this comment

ndelangen Oct 1, 2024

Choose a reason for hiding this comment

JReinhold Oct 1, 2024

Choose a reason for hiding this comment

nspector commented Oct 31, 2024

CLI: Migrate from `chalk` to `picocolors` #28262

CLI: Migrate from `chalk` to `picocolors` #28262

43081j commented Jun 17, 2024 •

edited

Loading

nx-cloud bot commented Jun 17, 2024 •

edited

Loading

JReinhold commented Sep 17, 2024 •

edited

Loading