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

fix: Log compiler errors in js config files #915

Closed
wants to merge 2 commits into from

Conversation

kf6kjg
Copy link
Contributor

@kf6kjg kf6kjg commented Oct 13, 2020

Before this change javascript config files that had compilation errors or that threw their own errors wouldn't report the error, making debugging a mite more difficult than it should be: instead you'd get a useless TypeError: Cannot read property 'errors' of undefined message. Now if the javascript config file tosses its cookies for whatever reason we can read what that reason was.

In my case specifically I was accessing a variable before declaring it. Getting

Could not parse lint-staged config.

ReferenceError: Cannot access 'result' before initialization

Please make sure you have created it correctly.
See https://github.com/okonet/lint-staged#configuration.

was a LOT more informative than

Could not parse lint-staged config.

TypeError: Cannot read property 'errors' of undefined

Please make sure you have created it correctly.
See https://github.com/okonet/lint-staged#configuration.

@iiroj
Copy link
Member

iiroj commented Oct 16, 2020

Thanks for the PR! Unfortunately it looks like you left some unlinted code in the commit (maybe you didn't run lint-staged).

Before this change javascript config files that had compilation errors or that threw their own errors wouldn't report the error, making debugging a mite more difficult than it should be: instead you'd get a useless "TypeError: Cannot read property 'errors' of undefined" message.  Now if the javascript config file tosses its cookies for whatever reason we can read what that reason was.
@kf6kjg
Copy link
Contributor Author

kf6kjg commented Nov 4, 2020

@iiroj Sorry for the delay, fixed. I'd created the initial patch using GitHub's online interface based on a patch-package patch I'd created for one of my projects, and yes that interface doesn't run the linter!

@codecov
Copy link

codecov bot commented Nov 4, 2020

Codecov Report

Merging #915 (1ef770d) into master (0ff2917) will decrease coverage by 0.16%.
The diff coverage is 93.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##            master     #915      +/-   ##
===========================================
- Coverage   100.00%   99.83%   -0.17%     
===========================================
  Files           18       18              
  Lines          609      615       +6     
  Branches       143      148       +5     
===========================================
+ Hits           609      614       +5     
- Misses           0        1       +1     
Impacted Files Coverage Δ
lib/index.js 98.61% <93.33%> (-1.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ff2917...1ef770d. Read the comment docs.

@okonet
Copy link
Collaborator

okonet commented Nov 13, 2020

and yes that interface doesn't run the linter!

We should ask GitHub to run pre-commit hooks even when their UI is used. That would be rad!

@kf6kjg
Copy link
Contributor Author

kf6kjg commented Dec 3, 2020

@okonet Status on merge-ability?

@okonet
Copy link
Collaborator

okonet commented Dec 3, 2020

I have zero time to look at. @iiroj could you review please? 🙏

@iiroj
Copy link
Member

iiroj commented Dec 4, 2020

This PR needs additional tests before it can merged.

I took the liberty of creating a new branch with a test that covers this case:
https://github.com/okonet/lint-staged/compare/js-config-errors

@kf6kjg feel free to pick the commits into your branch.

@iiroj
Copy link
Member

iiroj commented Dec 4, 2020

@kf6kjg thanks for the PR! I'll close this one and opened another one with your commit in it, along with tests: #935

@iiroj iiroj closed this Dec 4, 2020
@kf6kjg
Copy link
Contributor Author

kf6kjg commented Dec 4, 2020

Thank you for the finishing touch! Good to see it completed.

@kf6kjg kf6kjg deleted the patch-1 branch December 4, 2020 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants