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

fmt: preserve spacing in templates to allow for organizing code #374

Open
andradei opened this issue Dec 29, 2023 · 16 comments
Open

fmt: preserve spacing in templates to allow for organizing code #374

andradei opened this issue Dec 29, 2023 · 16 comments

Comments

@andradei
Copy link
Contributor

andradei commented Dec 29, 2023

They are useful to organize HTML tags, but the formatter removes all empty lines.

Example:

This

templ Form() {
	<a href="event/new">
		+ Create
	</a>

	<form method="post" action="/event/new">
		<label for="title">Title</label>
		<input id="title" name="title" type="text" required class="border rounded-lg block"/>

		<label for="date">Title</label>
		<input id="date" name="date" type="date" required class="border rounded-lg"/>

		<button type="submit">Create</button>
	</form>
}

becomes

templ Form() {
	<a href="event/new">
		+ Create
	</a>
	<form method="post" action="/event/new">
		<label for="title">Title</label>
		<input id="title" name="title" type="text" required class="border rounded-lg block"/>
		<label for="date">Title</label>
		<input id="date" name="date" type="date" required class="border rounded-lg"/>
		<button type="submit">Create</button>
	</form>
}

It would be great if at least 1 blank line was kept on those situations.

Environment

  • macOS 14.2.1
  • templ 0.2.476
  • go1.21.5 darwin/arm64
  • VSCode 1.85.1
@axispx
Copy link

axispx commented Jan 7, 2024

+1 It would help with the readability of the code when I'm working with a div soup 😄

@joerdav joerdav added enhancement New feature or request fmt NeedsDecision Issue needs some more discussion so a decision can be made labels Jan 30, 2024
@joerdav joerdav changed the title Formatter erasing blank lines between tags fmt: preserve spacing in templates to allow for organizing code Jan 30, 2024
@joerdav
Copy link
Collaborator

joerdav commented Jan 30, 2024

I've set this as needs decision, I personally don't have any reservations against this, open to other opinions.

@dropwhile
Copy link

I ran into this issue as well, and the default formatting was a bit unexpected. I too would prefer newline preservation.

Perhaps it could be an option, similar to how the vscode html formatter has the "HTML>Format: Preserve New Lines" toggle (which I believe is enabled by default)?

This would allow people the option for either, and help prevent code churn for those that are fine with the existing formatting.

@andradei
Copy link
Contributor Author

andradei commented Feb 2, 2024

I think the solution is rather simple:
If a dev wrote empty lines that gofmt would preserve, it's safe to assume they expect to keep them after save. If the don't like them, they'd be writing .templ files without those empty lines. Making the formatter similar to gofmt would keep things consistent and meet already set expectations and all cases/tastes are covered.

@joerdav
Copy link
Collaborator

joerdav commented Feb 2, 2024

What about if duplicate new lines were removed?

<div />


<div />

formats to:

<div />

<div />

@a-h
Copy link
Owner

a-h commented Feb 2, 2024

I think the goal of a formatter is to be consistent, but by retaining whitespace, you end up with oddities.

templ Form() {
	<a href="event/new">
		+ Create
	</a>

	<form method="post" action="/event/new">
		<label for="title">Title</label>

		<input id="title" name="title" type="text" required class="border rounded-lg block"/>

		<label for="date">Title</label>
		<input id="date" name="date" type="date" required class="border rounded-lg"/>

		<button type="submit">Create</button>

	</form>
}

Would we want to leave that as-is? There's two pairs of label/input, but they're not grouped. And there's an extra line at the end that doesn't add any value.

I think that if we're adding line breaks to show logical grouping, then we're actually missing a HTML element that makes that grouping explicit.

For example, the Hypermedia book groups with a <p> tag (see https://hypermedia.systems/a-web-1-0-application/).

        <p>
            <label for="first_name">First Name</label>
            <input name="first_name" id="first_name" type="text" placeholder="First Name" value="{{ contact.first or '' }}">
            <span class="error">{{ contact.errors['first'] }}</span>
        </p>
        <p>
            <label for="last_name">Last Name</label>
            <input name="last_name" id="last_name" type="text" placeholder="Last Name" value="{{ contact.last or '' }}">
            <span class="error">{{ contact.errors['last'] }}</span>
        </p>
        <p>
            <label for="phone">Phone</label>
            <input name="phone" id="phone" type="text" placeholder="Phone" value="{{ contact.phone or '' }}">
            <span class="error">{{ contact.errors['phone'] }}</span>
        </p>

I introduced a formatter into templ because in my commercial work, developers write HTML in all sorts of tortured ways, with some adding loads of pointless whitespace, others putting every attribute on a newline etc. etc.

I'm up for making the change to not remove extra whitespace if there's broad support, but if a silent majority are happy (or don't care), then I'd like to keep it, since there's value in the consistency that removing non-essential whitespace brings.

@andradei
Copy link
Contributor Author

andradei commented Feb 7, 2024

Thanks for being willing to make changes and for explaining the abuses to HTML files people make on your experience... I'm glad I don't run into that often.

It would be quite the lengthy task to have a formatter for sane grouping. Imagine taking into account every possible combination of HTML tags that can be grouped.

Would we want to leave that as-is?

I agree with @joerdav and have a strong opinion that "yes," keep it as is with one small change: respect 1 blank line but remove any subsequent ones.

developers write HTML in all sorts of tortured ways, with some adding loads of pointless whitespace, others putting every attribute on a newline etc. etc.

I kind of agree with the pointless whitespaces. But attributes on newlines could be allowed after some arbitrary number (like 5 attributes) or if one line would result in a certain length (maybe 80).


I'd propose this ticket takes care of the whitespace formatting only and another issue is created for attribute formatting, tag grouping, etc., if those things are worth discussing.

@joeyak
Copy link

joeyak commented Feb 17, 2024

It would be nice if it were an option for the formatter. I like how it compacts everything, but tbh it wouldn't be bad if it followed gofmt and preserved spaces as long as it's consistent.

@leaanthony
Copy link

+1 for this!
It would be really great if we could disable the formatter, so that Render just output the rendered template as is. This would work really well for testing, especially snapshot testing.

@joerdav
Copy link
Collaborator

joerdav commented Mar 13, 2024

@leaanthony I think the conversation here is mainly talking about the templ fmt command, rather than how the HTML comes out at the end. If you'd like to discuss more then it could be worth a new issue, though for snapshot testing I'd recommend just formatting the HTML after rendering, using something like https://github.com/a-h/htmlformat this is what we do in the templ tests!

@leaanthony
Copy link

Oh perfect! Thank you for replying 🙏 Also, what a fantastic project! 🚀

@smithamax
Copy link

I think the goal of a formatter is to be consistent, but by retaining whitespace, you end up with oddities.

I think following the same rules as gofmt would be more consistent, I can group lines in a func, why not in a templ. I guess a templ is a single expression so different, but I still want to break up my code

I think that if we're adding line breaks to show logical grouping, then we're actually missing a HTML element that makes that grouping explicit.

This seems equivalent to arguing that instead of whitespace in go code you should just create a new block ({ ... }), but this would be noisy and change the code structure when all we want to do is make the code more pleasant to read

@AlexanderArvidsson
Copy link

AlexanderArvidsson commented Mar 29, 2024

I'd like to chime in here to +1 the comment @andradei, where if there's a newline, it most likely is due to the developer wanting the organisation. Me, personally, use single newlines to group relevant code in my HTML (React in reality, which for me to be readable needs grouping due to the mixed conditionals and HTML).

The standard formatter for most modern (JS) web development is Prettier, which does respect single newlines. It collapses multiple newlines into a single one, which I believe is the absolute correct way to go. I'm not trying to compare a JS formatter with a Go formatter, but gofmt also follows this rule, and prettier works for HTML which the standard go template package uses. It only makes sense the same comparison is valid with Templ.

Finally, @smithamax brings up the most important point (in my opinion); consistency. Templ is inconsistent even within itself:

func getFullName(store *Store) string {
	user := store.Get("user") // Newline below was kept

	return user.FirstName + " " + user.LastName
} // Even the newline below here was kept

templ page(store *Store) {
	<div>
		<span>Welcome,</span> // Newline below was removed
		<span class={ "bold" }>{ getFullName(store) }</span>
	</div>
}

The formatter keeps the newline (a single newline, collapsed if multiple) in the function, but it removes the newline in the template. From my perspective, this inconsistency should be enough to argue that something needs to change.
If the formatter can respect newlines outside of the templates inside .templ files, it should respect newlines inside the templates as well.

I think that if we're adding line breaks to show logical grouping, then we're actually missing a HTML element that makes that grouping explicit.

I've never heard this argument in my career, but it is an interesting topic for sure. Since the HTML specification is odd how it considers the formatting of HTML, I wouldn't consider this relevant though. When you make a newline in HTML, the specification ignores all newlines and whitespacing (because of indentation), meaning how you format your code is only relevant within a single line in HTML.
I can't think of a single time where I've purposefully tried to align my HTML code with the logical grouping (as in, with newlines), visually. It's actually impossible to do in many cases, because CSS controls the layouting (mostly). If you use Flex, you shouldn't try and make your HTML code be positioning the same way as Flex, right?

@andradei
Copy link
Contributor Author

andradei commented May 29, 2024

Pinging to see if we're close to a solution here.
I'm leaning even more towards respecting the white-space and blank-lines the user might add (knowingly or not 😄 ). Editors nowadays remove trailing white-space and if someone wants to write some horrendous code with white-space and blank-lines alone, I think that should be preserved for everyone to see (including the programmer in the future 😆).

And perhaps add something similar to gofmt in the future.

@joerdav
Copy link
Collaborator

joerdav commented May 31, 2024

@a-h It seems we have a majority of users who are wanting to have whitespace preserved, or atleast collapsed to a single line. I'll move this to the implementation stage.

@joerdav joerdav added help wanted Extra attention is needed NeedsImplementation Needs implementation and removed NeedsDecision Issue needs some more discussion so a decision can be made labels May 31, 2024
@linear linear bot added Migrated and removed enhancement New feature or request Migrated help wanted Extra attention is needed NeedsImplementation Needs implementation fmt labels Sep 4, 2024
AlexanderArvidsson added a commit to AlexanderArvidsson/templ that referenced this issue Sep 12, 2024
@AlexanderArvidsson
Copy link

AlexanderArvidsson commented Sep 12, 2024

Hello hello, I've done some work on this and would appreciate if @joerdav and/or @a-h takes a look at my PR.
It will collapse multiple newlines into a double-newline (so you preserve 1 empty line between elements).

I'm sure I could add way more tests for this, but this shouldn't affect any existing codebase.
It could be good to try running my fork on a big codebase and see what changes Git reports!
Running it on my own (small) codebase showed no changes, but the idea is that this is backwards compatible (as in, it shouldn't modify people's existing code). This is however hard to guarantee just via unit tests.

AlexanderArvidsson added a commit to AlexanderArvidsson/templ that referenced this issue Sep 19, 2024
AlexanderArvidsson added a commit to AlexanderArvidsson/templ that referenced this issue Sep 20, 2024
AlexanderArvidsson added a commit to AlexanderArvidsson/templ that referenced this issue Sep 29, 2024
AlexanderArvidsson added a commit to AlexanderArvidsson/templ that referenced this issue Sep 29, 2024
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

No branches or pull requests

9 participants