-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
Change to serialize as HTML instead of XML #22
Conversation
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #22 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 102 129 +27
=========================================
+ Hits 102 129 +27
☔ View full report in Codecov by Sentry. |
test.js
Outdated
@@ -93,10 +92,25 @@ test('parse', async (t) => { | |||
.data('settings', {fragment: false}) | |||
.processSync('<title>Hi</title><h2>Hello world!') | |||
.toString(), | |||
'<html><head><title>Hi</title></head><body><h2>Hello world!</h2></body></html>', | |||
'<html xmlns="http://www.w3.org/1999/xhtml"><head><title>Hi</title></head><body><h2>Hello world!</h2></body></html>', |
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.
I think this is what jsdom
does: I don’t see it happening (document.documentElement.outerHTML
printing an xmlns
) in safari
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.
Good job
// Comment, text, fragment. | ||
if ('textContent' in node) { | ||
const div = document.createElement('div') | ||
div.append(node) |
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.
Should this set innerText
in case the content has tag-like text?
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.
The textContent isn't actually used! Both that field and innerText should exist together AFAIK
But, I think we don't need this check, we can make a div (or template per remco's Q), and append to it, for any remaining node in the last case (text, comment, or fragment)
Closes GH-21.
Initial checklist
Description of changes
This switches from the DOMs
XMLSerializer
to manually using DOM node’s APIs to serialize as HTML.That is because the XML syntax is not always compatible with the HTML syntax.
However, it is a significant, breaking, change.
Closes GH-21.