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

[Proposal] Swap Chalk for Kleur #520

Merged
merged 5 commits into from
Jun 22, 2020
Merged

Conversation

stramel
Copy link
Contributor

@stramel stramel commented Jun 19, 2020

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.

@stramel stramel requested review from FredKSchott and drwpow June 19, 2020 17:47
@stramel stramel self-assigned this Jun 19, 2020
@vercel
Copy link

vercel bot commented Jun 19, 2020

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
Copy link
Collaborator

drwpow commented Jun 19, 2020

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 Outdated Show resolved Hide resolved
Copy link
Collaborator

@drwpow drwpow left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

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

@@ -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] {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting... 🤔 Thanks for looking into it!

@drwpow
Copy link
Collaborator

drwpow commented Jun 19, 2020

🤔 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
Copy link
Contributor Author

stramel commented Jun 19, 2020

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
Copy link
Collaborator

drwpow commented Jun 19, 2020

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
Copy link
Contributor

lukeed commented Jun 19, 2020

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

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

@stramel
Copy link
Contributor Author

stramel commented Jun 19, 2020

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
Copy link
Owner

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
Copy link
Contributor Author

stramel commented Jun 19, 2020

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

@FredKSchott
Copy link
Owner

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.

@stramel stramel marked this pull request as ready for review June 20, 2020 19:41
@stramel stramel requested a review from a team as a code owner June 20, 2020 19:41
@lukeed
Copy link
Contributor

lukeed commented Jun 20, 2020

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
Copy link
Contributor

lukeed commented Jun 20, 2020

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

Copy link
Owner

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

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

@FredKSchott FredKSchott merged commit e7d67ff into FredKSchott:master Jun 22, 2020
@stramel stramel deleted the ms/kleur branch June 22, 2020 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants