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 empty fragment string #26

Merged
merged 1 commit into from
Dec 9, 2024
Merged

Conversation

aguingand
Copy link
Contributor

@aguingand aguingand commented Dec 7, 2024

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

I've come to an error trying to stringify an empty string when fragment: true is enabled.
It fails here because hast-util-to-dom returns an XmlDocument with documentElement: null. So there is 2 solutions to me, either :

  1. Handle the XmlDocument with documenElement: null in rehype-dom-stringify
  2. Or, always return a DocumentFragment in hast-util-to-dom even if there is no child nodes

I have done solution 1 in this PR along with a failing test. If it you think solution 2 is more appropriate, I can make a PR to hast-util-to-dom.

@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 Dec 7, 2024
@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (6d41f5f) to head (2f91bc5).
Report is 25 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #26   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          102       135   +33     
=========================================
+ Hits           102       135   +33     

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

@wooorm wooorm merged commit 418a992 into rehypejs:main Dec 9, 2024
5 checks passed

This comment has been minimized.

@wooorm
Copy link
Member

wooorm commented Dec 9, 2024

Thanks!

Not sure about 2. Perhaps? It sounds sensical as you say to return a fragment? And weird if it doesn’t?

@wooorm wooorm added the 💪 phase/solved Post is done label Dec 9, 2024
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Dec 9, 2024
@wooorm
Copy link
Member

wooorm commented Dec 9, 2024

released, thank you!

@aguingand
Copy link
Contributor Author

hast-util-to-dom always returning a DocumentFragment if fragment = true seems more consistent to me !

@wooorm
Copy link
Member

wooorm commented Dec 13, 2024

Perhaps this is the mistake? https://github.com/syntax-tree/hast-util-to-dom/blob/5f2adac8060e43669e14d88624267de6a569f9f0/lib/index.js#L141. I’d welcome an improvement there if you want to look at it!

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

Successfully merging this pull request may close these issues.

3 participants