-
-
Notifications
You must be signed in to change notification settings - Fork 361
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
Add remark-github-admonitions-to-directives
to list of plugins
#1293
Conversation
Signed-off-by: Luud Janssen <luud-@live.nl>
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1293 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 6
Lines 142 142
=========================================
Hits 142 142 ☔ 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.
Thanks for sharing @LuudJanssen!
Some thoughts:
- consider annotating the plugin with
Plugin<[], Root>
to specific the node type it accepts and returns https://github.com/incentro-dc/remark-github-admonitions-to-directives/blob/6c2915ec62b252752c5e1ddff164d1160b5e5f9b/src/plugin.ts#L8 type
exports need to be put ahead of any other exports in package.json docs: https://www.typescriptlang.org/docs/handbook/modules/reference.html#packagejson-exports source: https://github.com/incentro-dc/remark-github-admonitions-to-directives/blob/6c2915ec62b252752c5e1ddff164d1160b5e5f9b/package.json#L24-L30- Plug and play module systems (pnpm and yarn) need types to be specified as
dependencies
https://github.com/incentro-dc/remark-github-admonitions-to-directives/blob/6c2915ec62b252752c5e1ddff164d1160b5e5f9b/src/parse-github-alert-blockquote.ts#L1 - Any system which doesn't recognize
exports
will break when usingmain
, as there are no.jss
files, only.js
files https://github.com/incentro-dc/remark-github-admonitions-to-directives/blob/6c2915ec62b252752c5e1ddff164d1160b5e5f9b/package.json#L31
Hi @ChristianMurphy, thank you so much for your awesome suggestions. I've applied all of them and released a v1.0.3 with the changes. |
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.
Looks good!
Very minor, I'd recommend widening version ranges of dependencies to the full major range https://github.com/incentro-dc/remark-github-admonitions-to-directives/blob/f67299024f9b9b58f4250a00300433b6a1f13f21/package.json#L48-L50
E.G. "@types/mdast": "^4.0.0",
this allows package managers to deduplicate more.
I've seen more issues from confusion on having multiple copies of the same library living on a computer than from being a patch or two behind.
Hi @ChristianMurphy, great suggestion. Even though the package manager should resolve this for most cases, there's always that dependency that has a fixed patch version. I've loosened up the version ranges and released v1.0.4. |
Cool stuff Luud! |
Initial checklist
Description of changes
I created a plugin to turn GitHub's alert syntax to the directives syntax used by admonitions. I'm curious to hear what you think and if it fits the list 😊
Plugin: https://github.com/incentro-dc/remark-github-admonitions-to-directives