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

cmd: rebuild and refresh when a go file is changed in --watch mode #646

Open
Giftzwerg02 opened this issue Mar 27, 2024 · 17 comments
Open

Comments

@Giftzwerg02
Copy link

Giftzwerg02 commented Mar 27, 2024

I have tried the example given in the docs: https://templ.guide/commands-and-tools/hot-reload

When trying to comment-out for example the "/" route, and then saving the main.go file, I don't see any reloads in my terminal and the route also still exists (when I refresh in the browser it still sends me the page).

I didn't find any related issues here and it's unclear to me how to debug this since no error or anything is logged.

The only thing that maybe causes this (given that it works for others (?)) is, that I am developing on NixOS.

I am using the following flake.nix:

{
  description = "Go proj flake";

  inputs.nixpkgs.url = "github:nixos/nixpkgs";  
  inputs.flake-utils.url = "github:numtide/flake-utils";

  outputs = { self, nixpkgs, flake-utils, ... }:
    flake-utils.lib.eachDefaultSystem (system:
      let
        pkgs = import nixpkgs { inherit system; };
      in
      {
        devShells.default = pkgs.mkShell
          {
            buildInputs = with pkgs;[
                go
                templ
                gcc
            ];
          };
      }
    );
}

I use it via nix develop and run the given example command: templ generate --watch --proxy="http://localhost:8080" --cmd="go run ."

@joerdav
Copy link
Collaborator

joerdav commented Mar 28, 2024

Templ only watches templ files, I recommend using templ watch in conjunction with air, similar to my example here:

https://github.com/joerdav/shopping-list

@joerdav joerdav closed this as completed Mar 28, 2024
@Giftzwerg02
Copy link
Author

I see, I didn't know that; I thought it would also react to changes of go files since it is mentioned like this in the docs:

If the *.go files change, #3 and #4 must be ran.

Do they just mean the go-files generated by templ here?

@a-h
Copy link
Owner

a-h commented Mar 28, 2024

Yes, just the Go files generated by templ, as per:

func shouldIncludeFile(name string) bool {
if strings.HasSuffix(name, ".templ") {
return true
}
if strings.HasSuffix(name, "_templ.go") {
return true
}
if strings.HasSuffix(name, "_templ.txt") {
return true
}
return false
}

However, it would be no trouble to update it to monitor all *.go files... maybe that would be better. What do you think @joerdav?

@a-h a-h reopened this Mar 28, 2024
@joerdav
Copy link
Collaborator

joerdav commented Mar 28, 2024

We could, my worry is that theres entire projects based on watching Go projects. I can imagine the templ watch tool gaining more complexity as we add more features that air covers.

Though, we could have a crude watch feature that covers 80% of cases, then recommend other tools if theres more niche scenarios.

Maybe I'm a bit biased since I'm very much enjoying the air+templ DX!

@a-h
Copy link
Owner

a-h commented Mar 28, 2024

I think doing the 80%, and making sure we support the use of air and similar tools for advanced use cases is ideal.

I added the change to not do all files here: dd1ee1e but maybe I was being over-cautious with that...

I think I could change it to include *.go files (if the --cmd arg is also set) and not break anyone else's expected workflows. If you want to get air or something else to build the software, don't add the --cmd flag to templ generate and air can pick up the change.

@joerdav
Copy link
Collaborator

joerdav commented Mar 28, 2024

Makes sense, we could look at including a glob or regex param to help cover the 80%, unless you think that would be too much too soon?

@jackielii
Copy link
Contributor

I stumbled across this with lots of search. I was using air to reload, but I miss the browser auto refresh feature. Initially I used browser-sync's proxy mode. However, I just couldn't reliably get the reload event to sent: It has to be after the server is restarted. In a way, I much prefer a polling method.

templ generate --watch gets most of them correctly: it refreshes the browser, although not instantly, but at least I don't have to switch over and cmd+r and switch back.

I don't mind the polling interval to be shorter as well, I'm fine trading more cpu with refresh speed.

Thanks & Regards

@jackielii
Copy link
Contributor

Aha, actually found a pretty good enough solution:

templ generate --watch --proxy="http://localhost:8080" --cmd="wgo run ."

So wgo watches go files, and templ watches .templ files. All covered!

@jackielii
Copy link
Contributor

jackielii commented Mar 28, 2024

Can I make one small suggestion:

Add templ generate --watch --notify to notify default listening port, i.e. 7331 a reload event

This makes other watcher to send the browser refresh event down easily.

Use case: I have tailwindcss watching files and generating css. Although this is mostly covered by changes in *.templ, but it's always lagging behind. I.e.

  1. *.templ changes
  2. templ regenerates _templ.txt and tailwindcss regenerates output.css
  3. usually templ is faster so the pages reload without the output.css change

If this feature is added, I could easily do a wgo -file output.css temple generate --watch --reload

@jackielii
Copy link
Contributor

@a-h are you willing to accept a PR that adds templ generate --watch --notify to notify an update progmatically? Thanks

@a-h
Copy link
Owner

a-h commented Mar 30, 2024

Thanks for writing that up into a nicely written up proposal @jackielii.

This issue can remain focused on whether templ should also support hot reload for *.go files.

I'm currently thinking that it should include *.go files by default, but have an option of --ignore-non-templ-go-files to switch it off for people that are using air / wgo etc.

@jackielii
Copy link
Contributor

jackielii commented Apr 4, 2024

Thanks for writing that up into a nicely written up proposal @jackielii.

This issue can remain focused on whether templ should also support hot reload for *.go files.

I'm currently thinking that it should include *.go files by default, but have an option of --ignore-non-templ-go-files to switch it off for people that are using air / wgo etc.

I think there might be a problem with it:

Imagine if we include *.go files for restarting the --cmd. The sequence of events will go like so:

templ file change

  1. templ detects a .templ file change and re-creates _templ.txt
  2. templ proxy server sends an reload event and browser reloads
  3. browser requests the route and the go server responds with the newly generated _temple.txt

go file changes

  1. templ detects a .go file change and kills the --cmd process, templ send reload event?
  2. templ re-runs the --cmd process which is probably a go run process
  3. templ sends the browser reload event

So the question is should templ send the browser reload event on 1 or 3? Intuitively it should send reload event on step 3, but:

Even if templ didn't send the reload event, the browser will still send a request because the SSE connection will try to re-connect ref
Event if templ tries to send the reload event, the browser client might not be connected.

The above are my suspicions, I didn't dig into them. However, the former does happen. You can test it in my setup #667, I talked about my suspicion here: #596 (reply in thread)

Finally, there is another case already handled, but worth mentioning:

When the go code related part of a templ file changes, the _templ.go changes, so the go server needs to be restarted. This is handled in templ generate's --cmd, and can be handled by air to include all go files. But I did make the mistake by excluding *_templ.go in air.

Update: After a few tests, I think sending reload event in 1 or 3 in the go file changes will both work: When the reload event is sent to the browser, the proxy server should be connected, so there is no "reconnect" happening here. The reload will hang for a bit to wait for the server to become available.

@joerdav
Copy link
Collaborator

joerdav commented May 31, 2024

I agree that it would be fine to trigger based on go files changing in the case of --cmd. We also now have thorough documentation on how to use air to support the templ watch workflow: https://templ.guide/commands-and-tools/live-reload-with-other-tools

Do we still want to add this feature?

@joerdav joerdav added enhancement New feature or request cmd NeedsDecision Issue needs some more discussion so a decision can be made labels May 31, 2024
@joerdav joerdav changed the title cmd: templ generate watch doesn't reload after changing go-files cmd: rebuild and refresh when a go file is changed in --watch mode May 31, 2024
@a-h a-h mentioned this issue Aug 18, 2024
@joerdav
Copy link
Collaborator

joerdav commented Aug 19, 2024

I think we are in agreement that we should be able to watch more than just templ files.

I've also received feedback that some would like to watch on more than just go and templ files. Use cases include embeded css files that require a rebuild if changed.

My suggestion is to switch to one flag:

--watch-pattern or similarly named.

The default of it would be *.(\.go|\.templ).

Air users could override to *.\.templ.

And users who want to build on a css change could do *.(\.go|\.templ|\.css).

We could avoid using regex if we wanted, opt for some kind of comma separated glob instead.

@Ilyes512
Copy link

Ilyes512 commented Aug 25, 2024

I also was going through https://templ.guide/commands-and-tools/live-reload and thought it would watch all my .go files and was surprised it didn't work as (I) expected.

So I was about to open an issue, but first searched the existing issues using watch and found this issue.

I think its a good decision to focus on the templ generated files and so I fully agree with that, but my suggestion would be to clearly mention this on the above doc page and also state that it is a conscious decision (maybe even mention this issue).

edit:

I created a PR with a possible suggestion for the note @ #896 Feel free to edit it before merging of course.

@linear linear bot added Migrated and removed Migrated enhancement New feature or request cmd NeedsDecision Issue needs some more discussion so a decision can be made labels Sep 4, 2024
@sedyh
Copy link

sedyh commented Sep 6, 2024

+1 for making templ to watch go files.
I'm still confused how to use --watch and --notify-proxy together with wgo since both --watch and go run are blocking.

@melkishengue
Copy link

+1 would also like to have this feature, then i could only use templ to watch all my files and not use air anymore

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

7 participants