hot-reload feature: experience report

[veggiemonk]

hot-reload feature: experience report

[a-h]

Thanks for taking the time to try out the feature and write it up. It's a new feature, and quite complex, hence the rough edges. Your testing is very useful.

On Issue 1 - Race condition between the proxy and the server, it's that the backend server isn't available yet (rather than the proxy).

Opening the browser uses some backoff to wait for the proxy to be ready to connect. time.Second is probably a bit too long for an initial start time tbh.

func openURL(url string) error {
	backoff := backoff.NewExponentialBackOff()
	backoff.InitialInterval = time.Second
	var client http.Client
	client.Timeout = 1 * time.Second
	for {
		if _, err := client.Get(url); err == nil {
			break
		}
		d := backoff.NextBackOff()
		fmt.Printf("Server not ready. Retrying in %v...\n", d)
		time.Sleep(d)
	}
	return browser.OpenURL(url)
}

Seems like it might be a good idea to add a round tripper inside the proxy to do a similar retry as per this: https://stackoverflow.com/questions/57317383/repeating-an-http-request-multiple-times-inside-a-reverse-proxy

Using backoff instead of a sleep should be more reliable, and faster.

2 - Good catch, definitely should be a prefix of text/html

3 - The sending of event is triggered at the same time that the server is rebuilding which causes to miss the update.

Do you mean sending the SSE to the browser to cause it to reload? I think that adding the retry to the proxy would also resolve this, because the proxy would hold open the request, and keep trying until the backend is up and running. That should also avoid the time.Sleep.

if args.Command != "" {
	fmt.Printf("Executing command: %s\n", args.Command)
	if _, err := run.Run(ctx, args.Path, args.Command); err != nil {
		fmt.Printf("Error starting command: %v\n", err)
	}
	// Send server-sent event.
	if p != nil {
		p.SendSSE("message", "reload")
	}
}

4 - Mandatory use of --cmd flag which is not really documented what is expected/allowed.

Ah, I see there's a bug that truncates the arguments to the cmd by mistake. It's not supposed to be mandatory though, I think moving the send outside of the if statement is all that's required, like this:

if args.Command != "" {
	fmt.Printf("Executing command: %s\n", args.Command)
	if _, err := run.Run(ctx, args.Path, args.Command); err != nil {
		fmt.Printf("Error starting command: %v\n", err)
	}
}
// Send server-sent event.
if p != nil {
	p.SendSSE("message", "reload")
}

5 - Waiting too long.

Yes, that's not good! 😁

I'll pick these up when I get the chance, or if you're up for contributing, let me know and I'll hang on and review.

[veggiemonk]

By all means, go ahead @a-h

I hope it didn't come too harsh. I re-read what I wrote and I'm sorry if this was lacking empathy. It was late and I was getting straight to the point. 😅

Note sure if you saw the changes I tried: #145

Regarding 1. not sure what happened but it didn't work for me out-of-the-box.

Regarding 3. this is what I mean

                             nothing changed
templ -----------------------X---------------------
        ^    ^      ^    ^
        |    |      |     \ reloading browser
        |    |      |
        |    |       \_ sending SSE to browser
        |    |     
        |     \_ generate new files 
        |   
         \_ change to _.templ file

air   --------------------------------------------
                 ^     ^        ^
                 |     |        \ new build ready and serving
                 |     |
                 |      \_ building
                 |
                 \_ detect changes

[joerdav]

Closing as the hot-reload approach has drastically changed since this report.

[a-h]

Just to add some detail to this - the two step changes in performance are:

• fsnotify based watcher - #470
• The update to --watch mode so recompilation is only required if you change Go code - #366

There's a short video at #470

The other issues you raised have been resolved.

Thanks for spending your time to put together the report!