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

Add remark-merge-data to list of plugins #1410

Merged
merged 1 commit into from
Feb 27, 2025
Merged

Conversation

s-h-a-d-o-w
Copy link
Contributor

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and discussions and couldn’t find anything or linked relevant results below
  • I made sure the docs are up to date
  • I included tests (or that’s not needed)

Description of changes

Adds remark-merge-data to list of plugins.

One use case for this is to share configuration across chart definitions when creating charts using code, like in my article here (which uses remark-kroki): https://aop.software/blog/2025-01-31_evaluating-llrt/ (code that makes use of remark-merge-data)

@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 Feb 19, 2025
Copy link

codecov bot commented Feb 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (a185821) to head (5b7d1eb).
Report is 40 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1410   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          142       138    -4     
=========================================
- Hits           142       138    -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ChristianMurphy
Copy link
Member

Thanks for sharing @s-h-a-d-o-w!
Could you expand the readme a bit?
Perhaps including the markdown file it's intended to work with? Currently I don't really understand from the current docs what this is or why an adopter would use it.

@s-h-a-d-o-w
Copy link
Contributor Author

Thanks for the feedback! I agree that the README wasn't sufficiently clear. I wrote that while being very focused on my use case, so I didn't notice that it's lacking context.

What do you think? https://github.com/s-h-a-d-o-w/remark-merge-data/blob/36cb97274229e69073037eb1744f2ab42c7c33d7/README.md

@ChristianMurphy
Copy link
Member

The additional docs help!
It looks like they're on a fork? Not in the source of the plugin being added here.

@s-h-a-d-o-w
Copy link
Contributor Author

Nope, that was a PR. I waited for your approval before merging and releasing it. Which I have done just now.

Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Looks good!
A few thoughts

@wooorm wooorm merged commit 06ac0b5 into remarkjs:main Feb 27, 2025
7 checks passed
@wooorm wooorm added 📚 area/docs This affects documentation 💪 phase/solved Post is done labels Feb 27, 2025

This comment has been minimized.

@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Feb 27, 2025
@s-h-a-d-o-w
Copy link
Contributor Author

This check may not be needed https://github.com/s-h-a-d-o-w/remark-merge-data/blob/e996968112056045034a5b9a1bb14157aa80fb20/src/index.ts#L43-L48, isYaml could be used to create a discriminated union https://www.typescriptlang.org/docs/handbook/unions-and-intersections.html#discriminating-unions, which would allow this to be split https://github.com/s-h-a-d-o-w/remark-merge-data/blob/e996968112056045034a5b9a1bb14157aa80fb20/src/index.ts#L7-L30 and TypeScript to give an error if the incorrect data type is passed in

That's a great shout, thanks! (I released a new version with that just before.)

Would data truly be unknown? or could it be constrained to be JSON? https://github.com/sindresorhus/type-fest?tab=readme-ov-file#json

I did this but then I realized—people might get creative with what they use. For example, they could use a proxy to track usage. As long as the engine can read from it like it's an object, it's fine by me. On the flip side, I'm not sure that the stricter type would yield much benefit here. (But I wasn't aware is this being in type-fest, maybe it'll come in handy in the future. Thanks!)

https://github.com/s-h-a-d-o-w/remark-merge-data/blob/e996968112056045034a5b9a1bb14157aa80fb20/src/index.ts#L63-L66 This will only support hard-coded meta attributes, if you also want to support dynamic, you may want to look to see if https://github.com/remcohaszing/rehype-mdx-code-props#readme could be supported as well

Since this plugin can be used with other remark plugins, rehype would be too late? Also, I can't envision how this could be useful for selecting which blocks to merge. Finally, unless I misunderstand something, it's only dynamic at runtime, so I could only match the prop declarations—which seems to me essentially the same as matching strings.

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Mar 10, 2025

I did this but then I realized—people might get creative with what they use. For example, they could use a proxy to track usage. As long as the engine can read from it like it's an object, it's fine by me. On the flip side, I'm not sure that the stricter type would yield much benefit here.

I understand, and I see it a different way.
My thinking, if it isn't tested, it may not work.
You're right people will try random other types if they're allowed, and they will log issues if they don't work, that adds frustration for them and more work for you.
If you want to support proxy, add a test for it and add the type to the typing.
If a user wants another type to be supported, they can add a test and the type in a PR.

Since this plugin can be used with other remark plugins, rehype would be too late?

It would be if it is purely used as a remark plugin.
That said, from your example, it seems its focus is generating HTML? If so this could also become a rehype plugin (and perhaps should be)

I can't envision how this could be useful for selecting which blocks to merge. Finally, unless I misunderstand something, it's only dynamic at runtime, so I could only match the prop declarations—which seems to me essentially the same as matching strings.

That's fair, if you don't have a use case for it, don't add it.

@s-h-a-d-o-w
Copy link
Contributor Author

from your example, it seems its focus is generating HTML?

How so?

@ChristianMurphy
Copy link
Member

The main example you link to as motivating this is about generating rehype/HTML output https://aop.software/blog/2025-02-17_graphs-in-blogs/
And for Kroki or any other tool to work they generally need to be run through a renderer, presumably as part of the process you are doing.

@s-h-a-d-o-w
Copy link
Contributor Author

But the HTML output is generated by other plugins. This plugin only serves to share global data that is eventually used by those plugins.
(This just made me improve the example in the README though, to illustrate the data sharing aspect better. Maybe this makes it a bit clearer.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 area/docs This affects documentation 💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

3 participants