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

Need to refresh twice to reload routes #444

Open
poernahi opened this issue Jun 24, 2019 · 7 comments
Open

Need to refresh twice to reload routes #444

poernahi opened this issue Jun 24, 2019 · 7 comments

Comments

@poernahi
Copy link
Contributor

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 8f177df

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.

@yogthos
Copy link
Member

yogthos commented Jun 24, 2019

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

@yogthos
Copy link
Member

yogthos commented Jun 25, 2019

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
Copy link
Member

yogthos commented Jun 25, 2019

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

@poernahi
Copy link
Contributor Author

poernahi commented Jun 25, 2019

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.

poernahi/luminus-template-444@9df26e4

@yogthos
Copy link
Member

yogthos commented Jun 25, 2019

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.

@poernahi
Copy link
Contributor Author

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 :)

@yogthos
Copy link
Member

yogthos commented Jun 25, 2019

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. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants