[Proposal] Swap Chalk for Kleur #520

stramel · 2020-06-19T17:47:52Z

This PR replaces chalk with kleur.

@lukeed just recently released v4 which is generally faster than chalk and native ESM. https://twitter.com/lukeed05/status/1273320599008342016

Figured I would throw this out there as a PR to start the discussion. Only thing that I ran into was the project wasn't recognizing the typings provided by kleur

I slid some extra typings in here that we hadn't installed. I can pull this out if we decide to not go this direction.

vercel · 2020-06-19T17:47:57Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/2ri1bbybp
✅ Preview: https://snowpack-git-fork-stramel-ms-kleur.pikapkg.vercel.app

drwpow · 2020-06-19T17:50:55Z

Yess! I love PRs like this that reduce dependencies (especially w/ ESM).

I’m +1 on this but would love @FredKSchott’s thoughts as well.

src/commands/dev.ts

src/commands/paint.ts

src/rollup-plugin-catch-unresolved.ts

src/rollup-plugin-remote-cdn.ts

drwpow

Big fan of this! But again, happy to hear thoughts from @FredKSchott.

lukeed

Looks good, thanks :)

I'd say that the amount of color-nesting Snowpack has is just below the threshold where I'd start using kleur (main) instead. It makes writing chains easier, but the drawback is that it doesn't support tree shaking

lukeed · 2020-06-19T17:54:36Z

src/commands/paint.ts

@@ -47,22 +47,22 @@ export async function getPort(defaultPort: number): Promise<number> {
  return bestAvailablePort;
 }

-function getStateString(workerState: any, isWatch: boolean): [chalk.ChalkFunction, string] {
+function getStateString(workerState: any, isWatch: boolean): [colors.Colorize, string] {


Nice :) I'm glad that type came in handy for someone else haha

So since I couldn't get the typings provided to work, this actually just results in any currently 😢

Interesting. Is there something special about the TS config here? It works for me in multiple places

Do you have an example I can take a look at?

Also, @FredKSchott do you have any idea why it might not be picking up the typing?

Ah, funny enough, I was getting the type information from within JS files – not TS! 😆 microsoft/TypeScript#33079 (comment)

Interesting... 🤔 Thanks for looking into it!

drwpow · 2020-06-19T18:02:47Z

🤔 This is really interesting why the tests aren’t passing. You can see in test/integration/runner.js we are testing the stdout output of Snowpack to make sure it’s still behaving as expected.

Inside we have functions like this that replace things like 19.48 KB because we don’t care about that, and don’t want to fix those every time a stat changes (especially when it comes to install time in ms—how can that ever be the same?). So we replace certain parts of the string like so:

function stripStats(stdout) {
  // Need to strip leading whitespace to get around strange Node v13 behavior
  return stdout.replace(/\s+[\d\.]*? KB/g, '    XXXX KB');
}

And the tests are failing with:

- Expected
+ Received

- - snowpack installing...
- ✔ snowpack install complete.
-   ⦿ web_modules/         size       gzip       brotli
-     └─ preact/compat.js    XXXX KB    XXXX KB    XXXX KB
+ - snowpack installing...
+ ✔ snowpack install complete.
+   ⦿ web_modules/         size       gzip       brotli   
+     └─ preact/compat.js  19.48 KB   7.44 KB    6.86 KB

There’s something about colors that’s technically returning something different for stdout than chalk was, but not sure what yet. Might be worth digging in test/integration/runner.js and finding out.

stramel · 2020-06-19T18:04:50Z

There’s something about colors that’s technically returning something different for stdout than chalk was, but not sure what yet

I was afraid something like that would happen. If I get the go ahead on this PR, I can dig into it or maybe @lukeed can shed some insight on why this might be happening?

drwpow · 2020-06-19T18:07:47Z

Yeah please investigate! Unfortunately I don’t have time today to investigate myself.

If possible I’d like to figure out what the difference is in output before merging this PR. Maybe you can run the tests locally yourself, and/or test some local builds with this PR? Happy to help with that setup part if you need anything.

lukeed · 2020-06-19T18:11:20Z

@stramel @drwpow It's to do with ANSI code ordering for closing. The result is still the same, but since the kleur/colors module wraps the input string – instead of reiterating through a close-code order – it's technically is a bit different than kleur and chalk.

Here's an example:

kleur/colors: https://github.com/lukeed/kleur/blob/master/test/colors.js#L28
kleur: https://github.com/lukeed/kleur/blob/master/test/index.js#L26

Notice that in kleur/colors it's RB~text~BR and in kleur it's RB~text~RB

stramel · 2020-06-19T18:27:14Z

It also looks like the ANSI characters are being output in the string and our expected is not checking for those?

Output:

  console.log
    "- \u001b[1msnowpack\u001b[22m installing... \n✔ \u001b[1msnowpack\u001b[22m install complete.\u001b[2m\u001b[22m\n\n  ⦿ \u001b[1mweb_modules/        \u001b[22m\u001b[1m\u001b[4msize     \u001b[24m\u001b[22m  \u001b[1m\u001b[4mgzip     \u001b[24m\u001b[22m  \u001b[1m\u001b[4mbrotli   \u001b[24m\u001b[22m\n    \u001b[2m└─\u001b[22m solid-js/dom.js  \u001b[31m43.89 KB   \u001b[39m\u001b[32m10.74 KB   \u001b[39m\u001b[32m9.53 KB    \u001b[39m\n\n"

      at stripWhitespace (test/integration/runner.js:27:11)

Expected:

  console.log
    "- snowpack installing...\n✔ snowpack install complete.\n  ⦿ web_modules/        size       gzip       brotli\n    └─ solid-js/dom.js    XXXX KB    XXXX KB    XXXX KB"

      at stripWhitespace (test/integration/runner.js:27:11)

UPDATE:

Yep, if I have it strip the ANSI escapes from the original output it matches as expected. I'm not sure if we want to validate the formatting or not? @FredKSchott @drwpow

function stripAnsiEscapes(stdout) {
  // Regex from `ansi-regex`
  return stdout.replace(/[\u001B\u009B][[\]()#;?]*(?:(?:(?:[a-zA-Z\d]*(?:;[-a-zA-Z\d\/#&.:=?%@~_]*)*)?\u0007)|(?:(?:\d{1,4}(?:;\d{0,4})*)?[\dA-PR-TZcf-ntqry=><~]))/g, '');
}

FredKSchott · 2020-06-19T19:28:18Z

I've been using Chalk for so long, it's cool to see some new options in the space.

I don't feel too strongly either way, so if this is getting people excited then I'm +1 for that alone! Definitely conceptually aligned with the ESM-first approach, and love the idea of supporting other libraries in this as much as possible.

Note that we still have chalk in our dependency tree via ora, but that's only used in our install command and tbh it has historically caused more output problems than it solves. Now that most people aren't no longer running snowpack install directly, I'd like to remove ora anyway.

stramel · 2020-06-19T22:00:02Z

@FredKSchott @drwpow Do you want ansi escapes in the expected output?

FredKSchott · 2020-06-19T22:30:03Z

Lets keep escaping for now. If we ever convert these to true snapshot tests, then we won't have to worry about maintaining the ansi ourselves in expected-install files, and then I'd feel better about adding it back in.

lukeed · 2020-06-20T21:19:01Z

Just a head up that Node 13.0 thru 13.6 will resolve kleur incorrectly because they included different, experimental versions of the exports map parsing. All versions 13.7 and above work, as do all versions below 13.0 (6<=13)

lukeed · 2020-06-20T21:28:18Z

You might already know this though, as it's true for all packages with conditional, native ESM support.

Luckily there have been multiple versions of 13 that support this correctly -- and all of 13 is "end of life" come July

FredKSchott

LGTM but package-lock conflicts. Mind rebasing, deleting the package-lock & node_modules dir, and then reinstalling?

stramel requested review from FredKSchott and drwpow June 19, 2020 17:47

stramel self-assigned this Jun 19, 2020

vercel bot deployed to Preview June 19, 2020 17:47 View deployment

drwpow reviewed Jun 19, 2020

View reviewed changes

src/commands/dev.ts Outdated Show resolved Hide resolved

drwpow reviewed Jun 19, 2020

View reviewed changes

src/commands/paint.ts Show resolved Hide resolved

drwpow reviewed Jun 19, 2020

View reviewed changes

src/rollup-plugin-catch-unresolved.ts Show resolved Hide resolved

drwpow reviewed Jun 19, 2020

View reviewed changes

src/rollup-plugin-remote-cdn.ts Show resolved Hide resolved

drwpow approved these changes Jun 19, 2020

View reviewed changes

lukeed approved these changes Jun 19, 2020

View reviewed changes

vercel bot deployed to Preview June 19, 2020 17:59 View deployment

vercel bot deployed to Preview June 19, 2020 18:01 View deployment

vercel bot deployed to Preview June 20, 2020 19:35 View deployment

stramel marked this pull request as ready for review June 20, 2020 19:41

stramel requested a review from a team as a code owner June 20, 2020 19:41

stramel force-pushed the ms/kleur branch from 39b6e60 to 1eabadc Compare June 21, 2020 02:21

vercel bot deployed to Preview June 21, 2020 02:21 View deployment

FredKSchott approved these changes Jun 22, 2020

View reviewed changes

Michael Stramel added 5 commits June 22, 2020 00:40

Initial conversion

9dcc3cc

Add types for packages that have them

fa0e36d

a few missed re-orderings

e23372f

fix typing check

7c40765

strip ansi-escapes

81ea49b

stramel force-pushed the ms/kleur branch from 1eabadc to 81ea49b Compare June 22, 2020 05:41

vercel bot deployed to Preview June 22, 2020 05:41 View deployment

FredKSchott merged commit e7d67ff into FredKSchott:master Jun 22, 2020

stramel deleted the ms/kleur branch June 22, 2020 19:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Proposal] Swap Chalk for Kleur #520

[Proposal] Swap Chalk for Kleur #520

stramel commented Jun 19, 2020

vercel bot commented Jun 19, 2020 •

edited

drwpow commented Jun 19, 2020 •

edited

drwpow left a comment

lukeed left a comment

lukeed Jun 19, 2020

stramel Jun 19, 2020

lukeed Jun 19, 2020

stramel Jun 20, 2020

lukeed Jun 20, 2020

stramel Jun 20, 2020

drwpow commented Jun 19, 2020 •

edited

stramel commented Jun 19, 2020

drwpow commented Jun 19, 2020

lukeed commented Jun 19, 2020

stramel commented Jun 19, 2020 •

edited

FredKSchott commented Jun 19, 2020

stramel commented Jun 19, 2020

FredKSchott commented Jun 19, 2020

lukeed commented Jun 20, 2020

lukeed commented Jun 20, 2020

FredKSchott left a comment

[Proposal] Swap Chalk for Kleur #520

[Proposal] Swap Chalk for Kleur #520

Conversation

stramel commented Jun 19, 2020

vercel bot commented Jun 19, 2020 • edited

drwpow commented Jun 19, 2020 • edited

drwpow left a comment

Choose a reason for hiding this comment

lukeed left a comment

Choose a reason for hiding this comment

lukeed Jun 19, 2020

Choose a reason for hiding this comment

stramel Jun 19, 2020

Choose a reason for hiding this comment

lukeed Jun 19, 2020

Choose a reason for hiding this comment

stramel Jun 20, 2020

Choose a reason for hiding this comment

lukeed Jun 20, 2020

Choose a reason for hiding this comment

stramel Jun 20, 2020

Choose a reason for hiding this comment

drwpow commented Jun 19, 2020 • edited

stramel commented Jun 19, 2020

drwpow commented Jun 19, 2020

lukeed commented Jun 19, 2020

stramel commented Jun 19, 2020 • edited

FredKSchott commented Jun 19, 2020

stramel commented Jun 19, 2020

FredKSchott commented Jun 19, 2020

lukeed commented Jun 20, 2020

lukeed commented Jun 20, 2020

FredKSchott left a comment

Choose a reason for hiding this comment

vercel bot commented Jun 19, 2020 •

edited

drwpow commented Jun 19, 2020 •

edited

drwpow commented Jun 19, 2020 •

edited

stramel commented Jun 19, 2020 •

edited