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

Update log.js to support async reporter plugins #71

Merged
merged 4 commits into from
Nov 19, 2022

Conversation

steveszc
Copy link
Contributor

@steveszc steveszc commented Nov 16, 2022

Async reporters can be easily supported by awaiting the reporter function.

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Log.js can easily support sync reporter plugins by awaiting the reporter function. Currently the reporter function is called as a sync function.

Example: vfile-reporter-junit
https://github.com/kellyselden/vfile-reporter-junit/blob/master/index.js

Async reporters can be easily supported by `await`ing the reporter function.
@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Nov 16, 2022
@steveszc
Copy link
Contributor Author

Potentially related issue comment. Unclear if the decision to have reporter plugins be sync was an intentional design decision. #62 (comment)

Copy link
Member

@remcohaszing remcohaszing left a comment

Choose a reason for hiding this comment

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

I see why this is useful. The types need to be updated though.

ESLint 8 also added support for async reporters.

@steveszc
Copy link
Contributor Author

I am getting type errors on main when I run npm test.

Not sure I can get this PR passing until main is working...

@wooorm
Copy link
Member

wooorm commented Nov 17, 2022

@codecov-commenter
Copy link

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (e7fc5f9) compared to base (398ebe2).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head e7fc5f9 differs from pull request most recent head cf12f6d. Consider uploading reports for the commit cf12f6d to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #71   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines         2555      2555           
=========================================
  Hits          2555      2555           
Impacted Files Coverage Δ
lib/file-set-pipeline/log.js 100.00% <100.00%> (ø)
lib/index.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@steveszc
Copy link
Contributor Author

@wooorm Test added and passing

@wooorm wooorm merged commit 245bd28 into unifiedjs:main Nov 19, 2022
@github-actions

This comment has been minimized.

@wooorm wooorm added 🦋 type/enhancement This is great to have 💪 phase/solved Post is done labels Nov 19, 2022
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Nov 19, 2022
@wooorm
Copy link
Member

wooorm commented Nov 19, 2022

Thanks, released in 10.1.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done 🦋 type/enhancement This is great to have
Development

Successfully merging this pull request may close these issues.

4 participants