-
Notifications
You must be signed in to change notification settings - Fork 22
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: support flat config #161
feat: support flat config #161
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #161 +/- ##
==========================================
- Coverage 99.30% 99.22% -0.08%
==========================================
Files 15 17 +2
Lines 866 907 +41
Branches 73 74 +1
==========================================
+ Hits 860 900 +40
- Misses 6 7 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! So sorry for taking multiple weeks to get back to you on this - it's a good change and important for the plugin. Thank you very much for getting it started!
The general path you took here seems reasonable to me. Glad to see the actual changes to src/index.ts
are pretty straightforward. I left some requests inline about streamlining how the dual-emit (CJS/ESM) and dual-format (flat/legacy) support should look.
Also, per #91 (comment), requesting:
Document how to use them with flat config in the README.md
I think I'd made a note to get back to this PR once that was in and forgot to follow up 😅.
Co-authored-by: Josh Goldberg ✨ <[email protected]>
Co-authored-by: Josh Goldberg ✨ <[email protected]>
Co-authored-by: Josh Goldberg ✨ <[email protected]>
Hi, I have changed the code to apply your suggestion. I intentionally left the documentation section until the implementation is refined based on your review, which has now been added. Thanks a lot for your review. I have learned a lot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks a bunch @hyoban! 🙌
I only have some small, honestly non-blocking comments left. Do you have opinions on them?
If not, no worries, I've set a reminder to merge this on Wednesday. 🚀
@JoshuaKGoldberg I have no problem with your modification, but I don't have a computer with me now. You can modify and merge it directly. Or I will deal with it tomorrow. |
You got it, I'll go ahead and do that now so we can release today. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Figured I'd add a quick one here so code coverage doesn't drop. 🤷
🎉 This is included in version v0.11.0 🎉 The release is available on: Cheers! 📦🚀 |
PR Checklist
status: accepting prs
Overview
Some code are not very good, looking forward to your suggestions