fix(fmt): Preserve multiple newlines between elements (#374)

[AlexanderArvidsson]

Preserves multiple lines between elements, to allow organizing code better.
For reference and discussion, see #374.

templ fmt is inconsistent with itself, sometimes preserving newlines (collapsing) and sometimes not.
This aligns templ fmt with gofmt, which also preserves newlines (by collapsing multiple standalone newlines into one), which templ fmt also does in go functions.

This change should be backwards compatible (as in, it shouldn't modify existing code bases since they all should already strip all newlines). But the best way to guarantee that is to run this fork on a big codebase and observe what it does.
Other than that, this PR also includes a couple of tests for this, both for the parser and the formatter output. I can add more formatter tests if needed, in case we find some edge cases.

[AlexanderArvidsson]

I found a case that I'd like to include before potentially accepting this PR:

temp x() {
	<header></header>

	@c.Hero()
	<main class="max-w-7xl mx-auto px-8 mt-24 overflow-hidden"></main>
			
	<a></a>

	if true {
		<span></span>
	}
	<a></a>
}

The space should have been preserved between c.Hero and main and the if statement and a tag, but they were stripped.

[AlexanderArvidsson]

I also found that this throws an error in my own project, due to the first <a></a>. Says test_templ.go:32:61: string literal not terminated (and 8 more errors), but doesn't seem to throw anything (and formats correctly) when I run it as a format test.

package components

templ Test() {
  <div class="flex items-center gap-4">
    <a></a>



    <a>
    </a>
  </div>
}

---

EDIT: I've found that I've missed adding checks where there's normally a n == parser.SpaceVertical, it doesn't account for SpaceVerticalDouble. For example, this strips newlines into horizontal space:

	// Normalize whitespace for minified output. In HTML, a single space is equivalent to
	// any number of spaces, tabs, or newlines.
	if n == parser.SpaceVertical {
		n = parser.SpaceHorizontal
	}

This should also do the same for parser.SpaceVerticalDouble. This will fix the issue I was facing, since this affects the minified generated go code, and thus the unterminated string.

Also, this switch statement must include a parser.SpaceVerticalDouble to preserve indent:

		switch trailing {
		case SpaceNone:
			level = 0
		case SpaceHorizontal:
			level = 0
		case SpaceVertical:
			level = startLevel
		}

[AlexanderArvidsson]

@joerdav Before I go and preserve newlines for statements like if and for, I've decided to only focus on Elements and TemplElements.
Adding the trailing space logic to every statement is a bigger change that I think would only be fair if I got your opinion on first.

You can see how I added the logic to TemplElementExpression and let me know if I should do the same for if, for, etc.

I believe consistency is important, so I think preserving newlines should work as you expect for all types of statements or expressions, but let me know your thoughts.

[joerdav]

Hey @AlexanderArvidsson , thanks for all the time on this so far, I've looked at the code here and am happy with how you have approached this. I agree with you that due to consistency I think that we should support trailing spaces for all elements.

I think going ahead with this pattern is a solid approach.

One to think about is that the following code block may end up repeated a bit so could maybe be made into a function (or not since I suppose it isn't a lot of code):

		// Parse trailing whitespace after expression.
		ws, _, err := parse.Whitespace.Parse(pi)
		if err != nil {
			return r, false, err
		}
		r.TrailingSpace, err = NewTrailingSpace(ws, true)
		if err != nil {
			return r, false, err
		}

[AlexanderArvidsson]

One to think about is that the following code block may end up repeated a bit so could maybe be made into a function (or not since I suppose it isn't a lot of code):

		// Parse trailing whitespace after expression.
		ws, _, err := parse.Whitespace.Parse(pi)
		if err != nil {
			return r, false, err
		}
		r.TrailingSpace, err = NewTrailingSpace(ws, true)
		if err != nil {
			return r, false, err
		}

@joerdav The elementparser has a function addTrailingSpaceAndValidate which does almost the same thing, it also checks for voidElementCloser and then validates. I can put the addTrailingSpace portion of it in its own function and use that everywhere!

So far, I just followed the "don't repeat yourself more than twice" philosophy, and just copied that small piece of code twice. But for more uses, I would definitely put it in a function. I'll get on adding support for the rest of the elements and try and find all cases.

Also, tests seem to be failing, even locally (I think I had only ran the parser tests, not generate ones), so I'll get on fixing that as well.

[AlexanderArvidsson]

@joerdav I've went ahead and added trailing space logic to most parsers and added relevant tests.
It's quite a lot of additions, but they're all very similar.

Please take an extra look at how I unified the trailing space logic in addTrailingSpace via the interface TrailingSpaceSetter. I kept them separate to the WhitespaceTrailer due to the required pointer receiver, but there might be a better way.

Other than that, from my point of view, this is ready to be tested!

[joerdav]

Amazing, thanks! I've tested this out on a fairly large repo at my company and as you said there is no effect to existing code.

I also have read the trailing space interface logic, I think it is done sensibly!

One note I have is on the addTrailingSpace function. I can't see any usages of 2 of the return values. Which could also mean that this no longer needs to be a generic function: func addTrailingSpace(e TrailingSpaceSetter, pi *parse.Input, allowMulti bool) error.

Going further, I wonder if we need the interface at all? If the trailing space function looked like: func parseTrailingSpace(pi *parse.Input, allowMulti bool) (types.TrailingSpace, err error)

It could be used as:

	// Parse trailing whitespace after closing brace.
	r.TrailingSpace, err = parseTrailingSpace(pi, true)
	if err != nil {
		return r, false, err
	}

What do you think?

Also in general, I think it would be worth @a-h casting his eyes over this.

[AlexanderArvidsson]

Amazing, thanks! I've tested this out on a fairly large repo at my company and as you said there is no effect to existing code.

I also have read the trailing space interface logic, I think it is done sensibly!

One note I have is on the addTrailingSpace function. I can't see any usages of 2 of the return values. Which could also mean that this no longer needs to be a generic function: func addTrailingSpace(e TrailingSpaceSetter, pi *parse.Input, allowMulti bool) error.

Going further, I wonder if we need the interface at all? If the trailing space function looked like: func parseTrailingSpace(pi *parse.Input, allowMulti bool) (types.TrailingSpace, err error)

It could be used as:

	// Parse trailing whitespace after closing brace.
	r.TrailingSpace, err = parseTrailingSpace(pi, true)
	if err != nil {
		return r, false, err
	}

What do you think?

Also in general, I think it would be worth @a-h casting his eyes over this.

You're absolutely right! I originally did have uses for the other values, but as I learned more about the codebase I started removing them. I think your way is much cleaner. I'll make those changes :)
I wasn't a big fan of the interface anyway but at the time was the quickest way I thought of.

Good to hear that you didn't see any effect on the existing code! :)

[joerdav]

All good fair enough, I know the feeling! You go on a journey with the code and end up with designs based off previous iterations more than your current understanding.

And yep no issues with the existing code! That's 165 templates to be exact :)

[AlexanderArvidsson]

@joerdav @a-h This should be ready for a final look-through now! I've applied the previous suggestions, even went a bit further for the statements that enforce a breaking space afterwards (like if, for, switch, etc).