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

Testing issues with chalk@2 #59

Closed
coreyfarrell opened this issue Mar 3, 2019 · 5 comments
Closed

Testing issues with chalk@2 #59

coreyfarrell opened this issue Mar 3, 2019 · 5 comments

Comments

@coreyfarrell
Copy link
Contributor

I've tested cliui against newer dependencies, upgrading chalk to 2.x causes a test failure:

  1) cliui
       layoutDSL
         ignores ansi escape codes when measuring padding:

      AssertionError: expected [ Array(9) ] to deeply equal [ Array(5) ]
      + expected - actual

       [
         "  |"
      -  "  "
         "  __|   __|  |   |   _ \\"
      -  "  "
         "  |    |     |   |   __/"
      -  "  "
         " \\__| _|    \\__,_| \\___|"
      -  " "
         "                         "
       ]

I've tested the following script:

const chalk = require('chalk');
console.log(JSON.stringify(chalk.blue('line 1\nline 2').split('\n'), null, 4));

Result from chalk@^1.1.2:

[
    "\u001b[34mline 1",
    "line 2\u001b[39m"
]

Result from chalk@^2.4.2:

[
    "\u001b[34mline 1\u001b[39m",
    "\u001b[34mline 2\u001b[39m"
]

So the big difference is that chalk@2 adds ansi codes to the start/end of each line. Upgrading wrap-ansi to the latest didn't fix the testing errors, it actually caused more errors though I'm unsure if this is due to an issue with wrap-ansi or with cliui.

@bcoe
Copy link
Member

bcoe commented Mar 4, 2019

@coreyfarrell is introducing ansi codes to the beginning and end of each line an intentional change in chalk@2? It seems pretty innocent, I wouldn't think it would break anything.

I'm more concerned about the updated version of wrap-ansi causing test failures.

@coreyfarrell
Copy link
Contributor Author

chalk/chalk#92 looks intentional to deal with an OSX terminal bug.

Both issues need to be fixed so cliui can update everything, but wrap-ansi is a direct dependency which means that cliui controls which version is installed / prevents the bug from manifesting. chalk is a devDependency so cliui has to deal with whatever it's given.

I've opened chalk/wrap-ansi#32 to add a failing test case. That would fix padding -> preserves leading whitespace as padding and partially fix layoutDSL -> ignores ansi escape codes when measuring padding. I believe the second test case should be adjusted to expect the last line to be empty, the failing to strip trailing spaces inside color codes was a bug in wrap-ansi@2.1.0.

I have a feeling that the wrap-ansi fix will correct both issues.

@coreyfarrell
Copy link
Contributor Author

@bcoe I was correct. I tested cliui with all dependencies updated then applied chalk/wrap-ansi#33, no test failures. Once that is merged / released I'll submit a PR to update cliui. It will cause node.js 6 to be required but that should be fine since yargs already requires the same.

@bcoe
Copy link
Member

bcoe commented Mar 6, 2019

@coreyfarrell sounds good, will happily accept this patch.

@coreyfarrell
Copy link
Contributor Author

@bcoe my patch yesterday accidentally updated string-width to 4.1.0 which requires node.js 8, now fixed to only upgrade to string-width 3.1.0.

Bonus: once this is merged and yargs is updated npm i yargs will not install any duplicate packages (only one version of each dependency will be used).

@bcoe bcoe closed this as completed in #60 Apr 4, 2019
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

No branches or pull requests

2 participants