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

feat: replace listr with listr2 #852

Merged
merged 18 commits into from
Apr 28, 2020
Merged

feat: replace listr with listr2 #852

merged 18 commits into from
Apr 28, 2020

Conversation

iiroj
Copy link
Member

@iiroj iiroj commented Apr 21, 2020

This PR replaces listr with listr2.

The most notable change is that the task output is shown "inline" where the task is run, and it's not collapsed.

Most of the refactoring came from listr2 not adding the errors array to its context, but it was a good update in any case.

What do you think, @okonet?

@iiroj
Copy link
Member Author

iiroj commented Apr 21, 2020

Here's a successful task run (no collapsing):

❯ node bin/lint-staged.js
✔ Preparing...
✔ Running tasks...
  ✔ Running tasks for *.{js,json,md}
    ✔ prettier --write
  ↓ No staged files match *.js [SKIPPED]
✔ Applying modifications...
✔ Cleaning up... 

Here's a failure:

❯ node bin/lint-staged.js
✔ Preparing...
⚠ Running tasks...
  ❯ Running tasks for *.{js,json,md}
    ✖ prettier found some errors. Please fix them and try committing again.
    lib/resolveTaskFn.js
  ✔ Running tasks for *.js
    ✔ npm run lint:base -- --fix
↓ Skipped because of errors from tasks. [SKIPPED]
✔ Reverting to original state because of errors...
✔ Cleaning up... 

@iiroj
Copy link
Member Author

iiroj commented Apr 21, 2020

Here's a fancy screenshot
Screenshot 2020-04-21 at 20 42 58

@iiroj
Copy link
Member Author

iiroj commented Apr 21, 2020

I'm fine with not changing the way errors are displayed, but the other stuff is pretty nice.

@iiroj iiroj marked this pull request as draft April 21, 2020 17:45
@okonet
Copy link
Collaborator

okonet commented Apr 22, 2020

This looks cool to me. The only concern is how it's going to look like when there are more errors? Like 20-30 lines? Now, they are printed last but with this update they won't be anymore. Can you test it and let me know how you feel about it?

@iiroj
Copy link
Member Author

iiroj commented Apr 22, 2020

@okonet I'll try to generate a huge error log. It does seem that listr2 updates the output into the correct positions without issues.

In any case might be safer to print them out last, like it is now.

@okonet
Copy link
Collaborator

okonet commented Apr 22, 2020

I was thinking about writing our own printer using https://github.com/vadimdemedes/ink but I don't have much time for it at the moment and Listr also solves lots of other edge cases for us.

@iiroj
Copy link
Member Author

iiroj commented Apr 22, 2020

Well, it doesn't seem to handle a lot of output, so let's just forget about it. In this example it loses the initial steps, and the start of the output itself:

❯ npx lint-staged
                      prettier/prettier
       46:18  error  Replace `".js"".jsx"` with `'.js''.jsx'`                                        prettier/prettier
       55:21  error  Replace `"babel-loader"` with `'babel-loader'`                                      prettier/prettier
       58:19  error  Replace `"webpack_production"` with `'webpack_production'`                          prettier/prettier
       59:19  error  Replace `"webpack_development"` with `'webpack_development'`                        prettier/prettier
       73:27  error  Replace `"main"` with `'main'`                                                      prettier/prettier
       77:19  error  Replace `"initial"` with `'initial'`                                                prettier/prettier
       78:17  error  Replace `"vendor"` with `'vendor'`                                                  prettier/prettier
      108:21  error  Replace `"disabled"` with `'disabled'`                                              prettier/prettier
    
    ✖ 113 problems (113 errors, 0 warnings)
      110 errors and 0 warnings potentially fixable with the `--fix` option.
    

    Error while parsing /Users/iiro/git/iiro.fi/src/constants/fonts.js
    Line 4, column 33: Line 4: Unexpected token, expected ";"

      2 |   "https://fonts.googleapis.com/css?family=IBM+Plex+Sans+Condensed:400,600|IBM+Plex+Serif:300&display=swap"
      3 | 
    > 4 | export const IBM_PLEX_SERIF = ""IBM Plex Serif""
        |                                 ^
      5 | 
      6 | export const IBM_PLEX_SANS_CONDENSED = ""IBM Plex Sans Condensed""
      7 | 
    `parseForESLint` from parser `/Users/iiro/git/iiro.fi/node_modules/babel-eslint/lib/index.js` is invalid and will just be ignored
↓ Skipped because of errors from tasks. [SKIPPED]
✔ Reverting to original state because of errors...
✔ Cleaning up... 

@iiroj
Copy link
Member Author

iiroj commented Apr 22, 2020

This is a bit closer to how it's currently working:

❯ node bin/lint-staged.js
✔ Preparing...
⚠ Running tasks...
  ❯ Running tasks for *.{js,json,md}
    ✖ prettier --check
↓ Skipped because of errors from tasks. [SKIPPED]
✔ Reverting to original state because of errors...
✔ Cleaning up... 

✖ prettier --check:
Checking formatting...
lib/index.js
Code style issues found in the above file(s). Forgot to run Prettier?

Screenshot 2020-04-22 at 17 12 05

@iiroj
Copy link
Member Author

iiroj commented Apr 22, 2020

It would be pretty simple to allow showing task output in the same way even when tasks pass, thoughts?

@okonet
Copy link
Collaborator

okonet commented Apr 22, 2020

I think that would be a great addition.

@iiroj iiroj changed the title feat: replace listr with listr2 and print errors inline feat: replace listr with listr2 Apr 22, 2020
@iiroj
Copy link
Member Author

iiroj commented Apr 24, 2020

Here's some refactoring.

task failed without output (because it's not found):

Screenshot 2020-04-24 at 11 59 20

task failed with output:

Screenshot 2020-04-24 at 12 00 05

Killing and SIGINT will work the same way, pringint the code and possible output.

I could add --output or similar, that would then always print output in the same way.

@iiroj
Copy link
Member Author

iiroj commented Apr 24, 2020

Here's a run with --output:

Screenshot 2020-04-24 at 12 38 42

What do you think, @okonet? The extra line-break in the second output comes from the task output itself.

@okonet
Copy link
Collaborator

okonet commented Apr 24, 2020

What does output flag do?

@iiroj
Copy link
Member Author

iiroj commented Apr 24, 2020

@okonet --output makes lint-staged always print task ouput, like in the screenshot. What would be a better name?

@iiroj iiroj marked this pull request as ready for review April 24, 2020 11:53
@iiroj iiroj requested a review from okonet April 24, 2020 11:55
@iiroj
Copy link
Member Author

iiroj commented Apr 24, 2020

Hmm, adding more failure codes like ENOENT unearthed that on Appveyor at least one test fails differently:

--- × node -e "process.exit(1)" failed without output (FAILED).
+++ × node -e "process.exit(1)" failed without output (ENOENT).

@iiroj
Copy link
Member Author

iiroj commented Apr 24, 2020

@okonet do you think --verbose would fit better to "always show output, even when tasks succeed"? We already have the debug option, but maybe these wouldn't get mixed up.

@okonet
Copy link
Collaborator

okonet commented Apr 24, 2020

I believe verbose is a better name for what we’re trying to do. And as you said since we have debug flag it won’t be collided.

@iiroj
Copy link
Member Author

iiroj commented Apr 26, 2020

@okonet I also went ahead and bumbed Node.js v13 to v14 in the CI, since that was recently released and should be the next LTS version.

okonet
okonet previously approved these changes Apr 26, 2020
@iiroj
Copy link
Member Author

iiroj commented Apr 26, 2020

@okonet I removed eslint-config-okonet since it was failing Windows Node.js 14.

@okonet
Copy link
Collaborator

okonet commented Apr 26, 2020

Yeah sure I didn’t update for years. Not sure I want to use it anymore :)

@iiroj
Copy link
Member Author

iiroj commented Apr 26, 2020

@okonet can you also update the CI check config to expect Node 14 instead of 13?

@okonet
Copy link
Collaborator

okonet commented Apr 26, 2020

Which check do you mean? How do I do that?

@iiroj
Copy link
Member Author

iiroj commented Apr 26, 2020

This was referenced Apr 26, 2020
@okonet
Copy link
Collaborator

okonet commented Apr 26, 2020

Ah yeah I’ll do this from my computer

okonet
okonet previously approved these changes Apr 27, 2020
@okonet
Copy link
Collaborator

okonet commented Apr 27, 2020

Done

@iiroj
Copy link
Member Author

iiroj commented Apr 27, 2020

@okonet I found a few places where the quiet option is not respected (mostly errors), but let's resolve these in another PR, since they're not directly related to this PR.

EDIT: Well, I managed to fix it, let me push...

@iiroj iiroj linked an issue Apr 27, 2020 that may be closed by this pull request
@iiroj iiroj mentioned this pull request Apr 27, 2020
@iiroj iiroj removed a link to an issue Apr 27, 2020
@iiroj iiroj merged commit 885a644 into master Apr 28, 2020
@iiroj iiroj deleted the listr2 branch April 28, 2020 12:55
@github-actions
Copy link
Contributor

🎉 This PR is included in version 10.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@iiroj iiroj linked an issue Jun 24, 2020 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

How to show warnings?
2 participants