awmottaz / prettier-plugin-void-html

Use the void tag syntax
https://www.npmjs.com/package/@awmottaz/prettier-plugin-void-html
MIT License
54 stars 5 forks source link

Void tag followed by any non-whitespace character duplicates closing > #10

Closed f11xter closed 2 months ago

f11xter commented 9 months ago

Input

<br>a

Expected Output

<br>a

Actual Output

<br>>a

Same effect seen with <input> and other void tags that don't cause Prettier to insert a newline immediately after them (like <hr> does).

Does not occur when plugin is disabled.

package.json:

{
  "devDependencies": {
    "@awmottaz/prettier-plugin-void-html": "^1.2.0",
    "prettier": "3.1.1"
  },
  "prettier": {
    "plugins": [
      "@awmottaz/prettier-plugin-void-html"
    ]
  }
}
awmottaz commented 9 months ago

Confirmed, looking into this. Thanks for reporting the bug!

awmottaz commented 9 months ago

I did a bit of digging... this is going to be quite difficult to fix I'm afraid.

The way my plugin works is by setting the isSelfClosing property to false in the pre-processed AST for all nodes (except MathML and SVG, as per the spec). This is how I get <div /> to print as <div></div>, but it also means that void tags such as <br> will now be printed as <br></br>.

The (incorrect) assumption I made was that the resulting Doc put the entire closing tag portion that we now don't want (</br>) as the last item in the contents array for that node.

https://github.com/awmottaz/prettier-plugin-void-html/blob/c3a95e4bc7c0b411827dec693876d7e026a76dfc/prettier-plugin-void-html.js#L62-L64

As it turns out, this is not always true! Prettier has a feature where closing tags might put their final > on a newline adjacent to the content following the tag. They achieve this by excluding it from the print output of the br node, with the next node "borrowing" it. You can look at this function and some of its callsites if you're curious.

I'll need to think about this some more to see if there is a workaround that isn't "implement an entire HTML printer from scratch" 😅

awmottaz commented 9 months ago

For the purpose of documenting my progress here — I spent a couple of hours seeing if I could inspect and mutate the Doc during the print function. It got very complex and fragile (it makes many more assumptions about how the Doc is structured). Best I could get was this:

ℹ tests 72
ℹ suites 0
ℹ pass 42
ℹ fail 18
ℹ cancelled 0
ℹ skipped 12

The failed tests are caused by the extra space character that Prettier would put in front of the /> self-closing syntax:

<br />
   ↑

I haven't found a way to reliably remove that space. It's very dependent on how each element is printed and the contents of the element's attribute set. So I was able to format these correctly: <br>a, <br><br>, but a space remains in <input name="test" > instead of <input name="test">, for example.

The skipped tests are for the MathML and SVG markup. These do allow self-closing syntax. I have not found a way yet to avoid removing the self-closing syntax in those cases, since the context of that node type is missing from the Doc itself and Docs get printed recursively.

awmottaz commented 9 months ago

The working branch for this progress is here, in case anyone else wants to build on what I've done so far: https://github.com/awmottaz/prettier-plugin-void-html/tree/fix-double-closing-bracket

f11xter commented 8 months ago

This is my attempt at a solution. A fair bit more naive but maybe with enough knowledge of the potential shapes of printed it could work.

awmottaz commented 8 months ago

@f11xter thanks for looking at this!

Did you try testing your solution against trailing content other than a single character? A couple examples that I found tricky:

f11xter commented 8 months ago

No, but looking at my approach, I think it might make no sense anyway.

I remove the borrowed ">", but that breaks the isLeadingSpaceSensitive handling, so perhaps it'd be better to look ahead to path.node.next.isLeadingSpaceSensitive and, if true, find and remove the closing ">" of the opening tag from the current void tag.

A rushed implementation results in:

<!-- input -->
<br><div><div><div>a</div></div></div>

<br><br>

<!-- output -->
<br>
<div>
  <div><div>a</div></div>
</div>

<br
><br>

<!-- expected output (no-plugin behaviour with void tags manually corrected) -->
<br>
<div>
  <div><div>a</div></div>
</div>

<br><br>

which is not quite right but close-ish.

f11xter commented 8 months ago

Almost working implementation: https://github.com/f11xter/prettier-plugin-void-html/commit/8d4629b4a0bd0aa8b29af523a069028e85e0e94d

<br><inline-element> seems to be triggering a special case, leading to an extra newline that I haven't found an answer for.

I imagine this solution is similarly short-sighted but it's probably worth a look @awmottaz :)

f11xter commented 8 months ago

So I've been trying to walk through the code with <br><span></span> as an example and I think the control flow is as follows:

  1. printer-html evaluates root node and calls printChildren
  2. printChildren calls printBetweenLine on the br
  3. printBetweenLine calls preferHardlineAsLeadingSpaces on the span
  4. preferHardlineAsLeadingSpaces calls preferHardlineAsTrailingSpaces on the br
  5. preferHardlineAsTrailingSpaces returns true: return ... node.fullName === "br" ...
  6. printBetweenLine receives true and returns hardline (L130)
  7. printChildren receives hardline and pushes it to trailingParts (L217)
  8. printChildren returns:
return [
  ...prevParts,
  group([
    ...leadingParts,
    group([printChild(childPath, options, print), ...trailingParts], {
      id: groupIds[childIndex],
    }),
  ]),
  ...nextParts,
];

which prints a much more complicated version of

[
  [
    ['<','br']
  ],
  [
    [{type: 'line', hard: true}, {type: 'break-parent'}],
    ['>','<span','>','</span','>']
  ]
]

All this to say that the hardline is set outside of the code we've written. So, unless we can tweak the br's fields to prevent this, I don't know how easy a fix will be.

A question worth answering is how the flow differs without the plugin. I think there are some places where a branch evaluates node.isSelfClosing which may lead to the answer.

(Side note: I abhor the codebase's use of ternaries. Utterly unreadable.)