-
Notifications
You must be signed in to change notification settings - Fork 45
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
Implement reitit reloading #130
Conversation
Once merged, one would also need to update all route definitions, e.g. in modules and docs to take advantage of reloading. I could not find a way to enable reloading of routes while still keeping route definitions returning vectors, |
libs/deps-template/resources/io/github/kit_clj/kit/src/clj/web/handler.clj
Outdated
Show resolved
Hide resolved
4496338
to
862914a
Compare
Moved dev mode check in init-key for |
Just pushed up a new template with the changes. |
@markokocic Hi I wanted to try this. I didn't read through the Reitit docs on it but couldn't get any re-evaluations I did take effect. Do you need to re-eval the actual handler or does any function down the call chain suffice? (I would guess so) |
HI @gerdint , you need to re-evaluate only what you changed in repl, cider, nrepl for your changes to be picked up. Just saving the file does nothing. You also need ot make sure init-key function for your route returns a fn instead of vector, like for example in api: (defmethod ig/init-key :reitit.routes/api
[_ {:keys [base-path]
:or {base-path ""}
:as opts}]
(fn [] [base-path route-data (api-routes opts)])) |
Hmm, this is exactly what I (thought) I did. Will have another go. It would save a lot of cpu cycles not to have to |
I forgot to add that you have to be in the In prod mode, routes are cached. |
@markokocic I still haven't managed to get this to work. For instance, when following the instructions in the guestbook tutorial and changing and re-evaluating Is there anything else that is needed? Looking at https://github.com/metosin/reitit/blob/master/doc/advanced/dev_workflow.md it seems they refer to the routes as vars or by named routes? I am starting the app with the |
@markokocic So I upgraded metosin/reitit to 0.7.2 and used a var to reference the handler and now I get the updated handler upon re-eval. This makes sense but doesn't rely on the changes in this PR no? The metosin document referenced above doesn't seem to require it? |
Hm, looks like reitit added support for vars after this pull request was merged. This means in order to have a reloaded workflow you either have to keep using functions approach, like in PR, or switch to vars. Not sure if it's possible to combine these two approaches. It may be more of a question for reitit. I'll try to revisit this sometimes in the future. |
Well using vars is more of a temporary solution since it means routes won't be cached in prod right? Used re-evaluating the router on every request in dev seems to be the way to go and IIUC what this PR is supposed to do, just don't get it to work. I haven't really studied reitit so my understanding may be wrong. But I still think that what I wrote above should be reproducible by just following the guestbook tutorial. If it actually works this workflow should probably be mentioned in the tutorial as well. When starting using Kit I was also new to the reloaded workflow and it wasn't clear from the docs (at the time anyway) how to work most efficiently. Having to do a (reset) on every change is quite slow I think. |
Fixes #48 by implementing reloading of routes as recommended by
reitit
in https://github.com/metosin/reitit/blob/master/doc/advanced/dev_workflow.mdTo take advantage of reloading, one has to return a function instead of a vector in his route definition. Check
(defmethod ig/init-key :reitit.routes/api
as an example.The solution is backward compatible: it still accepts vectors as a route definition, but in that case no reloading.
The reloading is by default enabled only in
:dev
profile.@yogthos , @nikolap please review before merging. I did some testing, but I'm not sure if there is a better/cleaner way to hook it up in integrant.