a-h / templ

A language for writing HTML user interfaces in Go.
https://templ.guide/
MIT License
7.14k stars 236 forks source link

generator: Duplicate Import Statements in Generated Templates Cause go build Failure #775

Closed akdotio closed 1 week ago

akdotio commented 1 month ago

Issue Description:

When a user imports certain packages explicitly in a template file, the generated template may include duplicate import statements. This duplication causes go build to fail.

Affected Packages:

Issue Example:

package main

import (
    "github.com/a-h/templ"
)

type LayoutProps struct {
    Content templ.Component
}

templ Layout(props *LayoutProps) {
    <div>
        @props.Content
    </div>
}

In this example, the generated Go code from the template will have two import statements for the package "github.com/a-h/templ".

Resolution:

The template generator should include import statements only if they are not already present in the template file.

joerdav commented 1 month ago

Thanks for raising, this is definitely a confusing bit of templ, the transparent importing of packages in the generated files.

One thought I had while looking at #757 was maybe we should force the template author to import templ explicitly as you have done, the problem comes with the other packages, a user may be confused as to why suddenly they are required to import strings even though they aren't using it.

The less than ideal solution would be to hide all the std lib package usages in the templ package, so that generated templates only rely on templ. This would bloat the runtime package a bit, but would allow us to have only templ as a required import.

The benefit of this explicit importing of packages is that it is clearer what is happening, and less confusing when someone tries to import io and get an error. It would also make implementing the organize import action simpler, as the import sections would be identical between generated templates and the template itself.

A drawback would be that this is a breaking change, though there may be a way to make it backwards compatible.

@a-h do you have any thoughts on this?

akdotio commented 1 month ago

@joerdav I just opened a PR with a potential resolution for this issue. The changes aim to maintain the current behavior while preventing duplicate import statements in the generated code when users explicitly import the same packages in the template file.

I'm not entirely sure if this fix is related to https://github.com/a-h/templ/issues/757. If you could review the proposed changes and share your thoughts, it would be greatly appreciated.

Thanks!

joerdav commented 1 month ago

Yes, the PR you contributed should fix it as is, I went on a bit of a tangent about a change that would maybe be good and solve the problem in a different way, but probably should have some more thinking around it!

I'll take a proper look at your PR soon, thanks!

a-h commented 1 month ago

Thanks for this. I love that you not only saw the problem, but that you took the time to put together a solution. Much appreciated.

I haven't had chance to look into this PR properly (I'm prepping for speaking at BigSkyDevCon on Saturday), but thought I'd write down my first thoughts while they're fresh.

I think this change relates to https://github.com/a-h/templ/issues/757 - since both are about managing imports. When we implement the import feature, it will definitely change the import set within a template.

In terms of changing the Go code output, in any testing, we need to make sure that the LSP continues to work, and I'm not sure that behaviour is covered in the unit tests as I see it so far. As we output Go code in a *_templ.go file, we need to maintain a map from the source code position in templ, over to the Go code so that the LSP functions correctly. Any solution we have to both #757 and this needs to consider this.

I was thinking about putting the templ imports at the end of the file, but suspect that formatting would then move them back to the right place, and damage the mapping. IIRC, we format the output *_templ.go file.

I see that regexp is being used to find the imports, and was wondering whether https://pkg.go.dev/go/parser#pkg-functions or similar could/should be used instead. We're already using the Go parser to parse Go expressions in https://github.com/a-h/templ/tree/main/parser/v2/goexpression - so I was wondering whether we already have the information required to understand which packages are being used, but we're just not returning it from the functions in the templ goexpression package.

So, that's my currently unresolved thoughts. I'll be able to take a proper look next week, once I'm back from the conference.

a-h commented 1 week ago

This should be resolved by https://github.com/a-h/templ/pull/793 which organises imports and removes duplicates etc. during formatting.

a-h commented 1 week ago

Fix released in https://github.com/a-h/templ/releases/tag/v0.2.731