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: Void tag followed by any non-whitespace character duplicates closing > #14

Merged
merged 11 commits into from
Jul 18, 2024

Conversation

f11xter
Copy link
Contributor

@f11xter f11xter commented Jan 26, 2024

  • Remove void element opening tag end marker (<br><br) if next element borrows it
  • Mark void element as isSelfClosing = true after printing to prevent parent from adding unwanted line breaks
  • When printing non-void element borrowing end marker from void element, mark void element as isSelfClosing = false to prevent adding /> to void element

Closes #10

f11xter and others added 7 commits January 15, 2024 00:15
Prettier always adds a newline after `<col>`, `<hr>`, `<param>`, `<source>` and `<track>` elements, no matter what comes after them.

Relates to awmottaz#10
(Probably) brittle solution.

TypeScript doesn't recognise the `path.node.prev` property.

Relates to awmottaz#10
`<br>` triggers a special case where an extra newline is added
@f11xter
Copy link
Contributor Author

f11xter commented Feb 27, 2024

Polite nudge @awmottaz :)

@awmottaz
Copy link
Owner

Yes, sorry, thanks for being patient. Life happens 😅

I do want to look at this very soon! Haven't lost track of it

@Kaleidosium
Copy link

Kaleidosium commented May 10, 2024

Any updates on this? This plugin will be very useful since Svelte removed self-closing tags due to it being incompatible with HTML. sveltejs/svelte#11052

@awmottaz
Copy link
Owner

Apologies for being so slow here — it's still on my radar, just haven't found the time to verify @f11xter 's fix yet.

@awmottaz
Copy link
Owner

@f11xter omg thank you for waiting on this! I am finally finding some time to look more deeply at this PR. I have a couple questions, I'll drop comments on the diff.

In the meantime, I just released v1.6.0 of this plugin. Would you mind updating your branch to the latest from main?

Copy link
Owner

@awmottaz awmottaz left a comment

Choose a reason for hiding this comment

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

This is looking great! Just a couple of questions and things to add, and I'll happily merge this in. Thanks so much!

Comment on lines +32 to +45
{ el: "area", hasTrailingNewline: false },
{ el: "base", hasTrailingNewline: false },
{ el: "br", hasTrailingNewline: false },
{ el: "col", hasTrailingNewline: true },
{ el: "embed", hasTrailingNewline: false },
{ el: "hr", hasTrailingNewline: true },
{ el: "img", hasTrailingNewline: false },
{ el: "input", hasTrailingNewline: false },
{ el: "link", hasTrailingNewline: false },
{ el: "meta", hasTrailingNewline: false },
{ el: "param", hasTrailingNewline: true },
{ el: "source", hasTrailingNewline: true },
{ el: "track", hasTrailingNewline: true },
{ el: "wbr", hasTrailingNewline: false },
Copy link
Owner

Choose a reason for hiding this comment

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

How is hasTrailingNewline determined for each tag? Is this defined in the Prettier source somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tbh I don't remember how I figured that out, but there's a decent probability it was just through running prettier without the plugin and observing the results.

I think the answer lies in printChildren's call to printBetweenLine but I've failed to trace it through to that conclusion.

Copy link
Owner

Choose a reason for hiding this comment

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

Okay, no worries. I'll just add a comment here saying something like that

test.js Outdated Show resolved Hide resolved
test.js Show resolved Hide resolved
@awmottaz
Copy link
Owner

Thanks again @f11xter — I'm ready to approve and merge this thing, looks like you just need to fix the linting issue (you can run npm run lint locally to check) and we're good to go!

@f11xter
Copy link
Contributor Author

f11xter commented Jul 17, 2024

Thank you so much for looking at this and I'm glad to have helped :)

@awmottaz awmottaz merged commit ef1ac04 into awmottaz:main Jul 18, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Void tag followed by any non-whitespace character duplicates closing >
3 participants