go-app icon indicating copy to clipboard operation
go-app copied to clipboard

Update of wasm binary doesn't trigger update of cached app

Open inliquid opened this issue 4 years ago • 20 comments

So here is very simple scenario

There is a message in main function

fmt.Println("Launching wasm app")

it's compiled and loaded, message appears in console.

Now change it to

fmt.Println("Launching wasm app 1")

Put cursor in address bar, press enter - nothing happens. Then press Ctrl-F5 (force reload), new message appears. Then try pressing Ctrl-R (regular reload), old message will come back, same if you put cursor in address bar and press enter - old message is there.

Tested in latest Firefox and Chrome on desktop.

inliquid avatar Jan 22 '21 14:01 inliquid

From further experiments it was found that in order to trigger application update you need:

  1. Rebuild wasm binary
  2. Rebuild and restart server
  3. Close browser tab and reopen it again with loading application page

inliquid avatar Jan 23 '21 13:01 inliquid

1 and 2 are normal. 3 should not be required.

maxence-charriere avatar Jan 23 '21 13:01 maxence-charriere

Can you show your main for the server ?

maxence-charriere avatar Jan 23 '21 13:01 maxence-charriere

I think it shouldn't be required to recompile and restart the server, forcing to close open connections, background jobs, and generally stopping the service, etc, when changes are only on client side such as cosmetics. Instead, server could check last modification date of the wasm file and rebuild app.js if needed.

inliquid avatar Jan 23 '21 13:01 inliquid

server is very generic:

package main

import (
	"log"
	"net/http"

	"github.com/maxence-charriere/go-app/v7/pkg/app"
)

// The main function is the entry of the server. It is where the HTTP handler
// that serves the UI is defined and where the server is started.
//
// Note that because main.go and app.go are built for different architectures,
// this main() function is not in conflict with the one in
// app.go.
func main() {
	// app.Handler is a standard HTTP handler that serves the UI and its
	// resources to make it work in a web browser.
	//
	// It implements the http.Handler interface so it can seamlessly be used
	// with the Go HTTP standard library.
	http.Handle("/", &app.Handler{
		Name:        "Hello",
		Description: "An Hello World! example",
		Resources:   app.LocalDir("../web"),
		Styles:      []string{"/web/spectre.css"},
	})

	err := http.ListenAndServe(":8000", nil)
	if err != nil {
		log.Fatal(err)
	}
}

inliquid avatar Jan 23 '21 13:01 inliquid

1 and 2 are normal. 3 should not be required.

However, I found it's required.

inliquid avatar Jan 23 '21 13:01 inliquid

It is required because pwa require to cache resources for offline mode. Whats is indicates that there is an update is a diff on the service worker. Service worker is generated by the handler depending on a version number that is automatically generated if not specified. Server does not need to be recompiled but relaunched in that case. You will alway have to recompile wasm binary if there is changes.

maxence-charriere avatar Jan 23 '21 14:01 maxence-charriere

Server can monitor modification date and update service worker without the need to restart. Right now it compiles template-based app.js once at startup. Template can be recompiled. Needing to restart a whole server for some (maybe very little) change on a client side is too much.

inliquid avatar Jan 23 '21 15:01 inliquid

Second thing is a requirement to reopen the tab, but it involves JavaScript, which I'm not expert on, but from some high level research I saw that it should be possible with some fine-tuning of service worker.

inliquid avatar Jan 23 '21 15:01 inliquid

There is no requirement to reopen a tab. The update when there is a new version is not instant. You can track the behavior by opening the browser console. Just need to reload.

If you want a visual on when an update is available, you can track when an app is updated in the browser by implementing the Updater interface. See https://go-app.dev/lifecycle#listen-for-updates.

maxence-charriere avatar Jan 23 '21 15:01 maxence-charriere

There is no requirement to reopen a tab. The update when there is a new version is not instant. You can track the behavior by opening the browser console. Just need to reload.

This is not the case. I tested several times and clearly saw that nothing is updated unless you reopen the tab.

inliquid avatar Jan 23 '21 15:01 inliquid

Thats is weird. I code everyday on the top of go-app without having this. Either on firefox or chrome. Could you tell me more about your deployment/build process ?

maxence-charriere avatar Jan 23 '21 15:01 maxence-charriere

Maybe make a tree at the package root and copy paste it below.

maxence-charriere avatar Jan 23 '21 15:01 maxence-charriere

Yea, you're right, I restarted browser and now I see new message appearing in the console. Not sure why it wasn't working. изображение

inliquid avatar Jan 23 '21 17:01 inliquid

Why is it closed? The issue is still there, I repeat myself:

I think it shouldn't be required to recompile and restart the server, forcing to close open connections, background jobs, and generally stopping the service, etc, when changes are only on client side such as cosmetics. Instead, server could check last modification date of the wasm file and rebuild app.js if needed.

Server can monitor modification date and update service worker without the need to restart. Right now it compiles template-based app.js once at startup. Template can be recompiled. Needing to restart a whole server for some (maybe very little) change on a client side is too much.

inliquid avatar Jan 26 '21 10:01 inliquid

You dont need to recompile the server, just launch it again.

Detecting a change in the wasm file would require to do a check on wasm file each time the service worker is requested (every page load). It would add an overhead that would have an impact on performances. This would be even worse in the case where you chose to host app.wasm in a remote bucket like s3 since it would have proxy the file in order to make a diff. Since performances can have a direct impact on cloud infra bill, I don’t want to force this in the default package behavior.

You have to recompile the wasm file anyway if you have a client modification. You could include relaunching the server in a makefile command that handle all building/launching the pwa and its server.

maxence-charriere avatar Jan 26 '21 11:01 maxence-charriere

I could be wrong so feel free to reopen if you still think a better job can be done on this :).

maxence-charriere avatar Jan 26 '21 12:01 maxence-charriere

If you decide to check that file on every call to ServeHTTP, it might have some performance penalty due to that additional check, but from my experience it will be negligible. So we could measure and decide if it worth it.

Another solution could be to check modification date on a time basis, like each 1,5,10 seconds or even more and recompile template when changes occur. In this scenario the weight of additional check will be even less noticeable as it's basically just an RWMutex check, and likelihood of serving at the same moment as the template recompiles is very small, and even if it happens, update of an app is rare, and this is better than restarting whole server.

There could be other solutions which we are not aware right now, such as idk, a completely different approach to building app.js?

Once again, I believe that restarting a server, which may perform some background jobs, forcing to close open and kept alive connections or websockets, and so on, is quite unfair price for let's say updating a cosmetics of a client part, such as changing some colors or whatever.

I could be wrong so feel free to reopen if you still think a better job can be done on this :)

As you closed it, I don't have a permission to reopen.

inliquid avatar Jan 26 '21 12:01 inliquid

I dealt with this with a wrapper which periodically checks the modification / hash (and a sync.RWMutex, as you say, @inliquid)

https://gist.github.com/tiedotguy/b8a29519c4690b45f89c003a37e36402

It's a bit more complicated in reality 'cause I've got some fsnotify junk in there, but I don't like that code, and it doesn't contribute anything, so I've stripped it.

tiedotguy avatar Feb 14 '21 10:02 tiedotguy

Thanks to Context.AppUpdateAvailable() this should now longer be an issue - I've implemented an update notification in https://github.com/pojntfx/bofied/blob/main/pkg/components/update_notification.go and I think lofimusic.app is also doing it like so (see https://github.com/maxence-charriere/lofimusic/blob/a99afd5037e01952e33fb1122e76dd8263888e79/bin/lofimusic/radio.go#L76-L78) :)

pojntfx avatar May 07 '21 22:05 pojntfx