luminus-template icon indicating copy to clipboard operation
luminus-template copied to clipboard

Need to refresh twice to reload routes

Open poernahi opened this issue 6 years ago • 7 comments

The current template requires reloading page twice before any route change takes effect.

Steps to reproduce:

  1. Make change to routing data and save file
  2. Refresh page

Expected: New routing data works

Observed: Old routing data is still in effect. When page is refreshed again, new routing data will take effect.

Analysis: According to the logs, the first refresh did trigger namespace recompile, but somehow the old handler is still used to process the request. The second refresh does not trigger recompile, but the new handler is used.

I suspect this has something to do with commit 8f177df4e994aeea1e48af299ce1918b58256261

I rolled back handler/app from mount/defstate to plain old function and the issue is fixed. I am not sure if this has something to do with reitit or not.

Additional information: I am using immutant as the http server. This should not make a difference though.

poernahi avatar Jun 24 '19 10:06 poernahi

Thanks for the report, I'll try take a look at this in the near future.

yogthos avatar Jun 24 '19 13:06 yogthos

I tried switching app to be a function, but that doesn't seem to be working locally. Would you happen to have an example with the change. If that works, that seems like it would be the simplest solution.

I tried the following locally:

(defn app [] ...)

and

(mount/defstate ^{:on-reload :noop} http-server
  :start
  (http/start
    (-> env
      (assoc  :handler (#'handler/app))
        (update :io-threads #(or % (* 2 (.availableProcessors (Runtime/getRuntime)))))
        (update :port #(or (-> env :options :port) %))))
  :stop
  (http/stop http-server))

yogthos avatar Jun 25 '19 00:06 yogthos

Also, as a workaround it looks like calling (user/restart) will reload things reliably.

yogthos avatar Jun 25 '19 00:06 yogthos

Reproduction with the fix below. I think this is what the template generated before 8f177df, so this is just a simple rollback. There may be a better way to resolve this with while still using defstate, but I have not dug deep enough to understand the reasoning behind this issue.

https://github.com/poernahi/luminus-template-444/commit/9df26e4528b4c40059a83d40df1915019328ae88

poernahi avatar Jun 25 '19 03:06 poernahi

Ah perfect, looks like (middleware/wrap-base #'app-routes) is what's causing the reload to work properly. Unfortunately, this will not automatically reload the middleware in wrap-base and you'd have to call (user/restart) to reload it. However, it does look like it's an overall improvement, so I pushed out an updated the template with the partial fix.

yogthos avatar Jun 25 '19 04:06 yogthos

I see.. We can pass in the handler var instead of its value.

Thanks! Routes probably change much more often than the base middleware, so this is a big improvement :)

poernahi avatar Jun 25 '19 04:06 poernahi

Yeah that's what I was thinking, and since there's a way to reload middleware from the REPL that seems like a decent compromise. :)

yogthos avatar Jun 25 '19 13:06 yogthos