Almost complete rewrite

[licht1stein]

Hi @weavejester, thanks for all you do!

I have tried using ring-refresh in a project with reitit and it didn't work, so I started investigating and making it work and as a result what I got is basically a complete rewrite of the original library 🙈

I would be grateful if you could take a look. What has changed:

• the js script now uses fetch to get the last save timestamp from endpoint and then compares it to the page loading time
• no watchers or state — each time the endpoint is triggered we check for .lastModified of all files in the watched dirs and return the highest value
• no compojure - just handle the :uri of the request to see if the __source_changed endpoint is called and substitute the handler
• no ring-params and no params
• inject the script before the ending </head> to fix Injecting the refresh script right after HEAD breaks utf-encoded pages #6
• converted from project.clj to deps.edn

It seems to work ok but I'm sure I must have missed something important, so I'd be grateful for input.

[weavejester]

Thanks for the PR. Before this can be merged we'll need to split it up into smaller PRs that each contain a single fix or feature, as it's very hard to review PRs that combine several changes into one. As far as I can see, the separate changes this PR encompasses are:

1. A switch from project.clj to deps.edn. Unfortunately this removes the capability to deploy to Clojars, so this change should be reverted.
2. Removing the Compojure dependency
3. Removing the Watchtower dependency in favour of iterating through the tree
4. Changing the JS to use some sort of atom rather than a parameter. (Is this safe to use with multiple tabs?)
5. Formatting changes (e.g. if-let to when-let)
6. Fixing issue Injecting the refresh script right after HEAD breaks utf-encoded pages #6.

[licht1stein]

Thanks, will do!

[licht1stein]

I have a question about this:

4. Changing the JS to use some sort of atom rather than a parameter. (Is this safe to use with multiple tabs?)

I don't think I understand the part about the atom. From the JS standpoint there are two changes:

1. Switched the function to use fetch in place of XMLHttpRequest (because of CORS issues).
2. Instead of sending the timestamp and receiving true or false, send a simple GET request and receive the server timestamp, then compare in the js function (so the back-end is now only responsible for sending the last save timestamp, no logic there).

So in terms of logic it stayed pretty much the same as in your original function, but on my side it stopped breaking because of CORS.

I'm sure I must be missing something and would be grateful for a tip. In any case I'll play with my version of the library for another week or two to make sure that everything keeps working, and the prepare several PRs as you said.

[weavejester]

That sounds reasonable. You're not missing anything; I skimmed the code as I didn't have time to do an in-depth review, but I also wanted to give you a rough idea of how the PR should be split up without making you wait. Your explanation of the JS code changes is appreciated, and sounds like an improvement on the original design.

[agorgl]

My 5c on this:
It would be nice if the refresh functionality can be triggered manually too instead of relying on filesystem watchers.
This way one can use reload+refresh without saving the actual file that contains the namespace changes, which is closer to the repl driven development that relies on selectively evaluating some stuff.

[licht1stein]

@agorgl this isn't that hard to implement, actually. But I don't quite see a use case. So you evaluate some code and then run a separate command to trigger refresh?

Or you want any eval to trigger refresh?

[licht1stein]

After thinking some more I realized that this feature is already implemented in my fork.

All you need to do to trigger a refresh is

touch -m -t TIMESTAMP src

for example. This would update the last modified date and refresh will trigger automatically.

[agorgl]

The thing with the current approach that ring-refresh uses, is that it is not very aligned with the normal repl evaluation workflow that Lisps/Clojure uses. It is dependent on filesystem changes that might happen in any part of your source code. They achieve similar things but the selective evaluation model that Clojure uses is way more controlled and fine grained.

[agorgl]

The main benefit is the control of what to evaluate/reload. One might want to eval only part of some refactor/wip code while saving the code, without having to worry that it will mess-up his live application instance

[licht1stein]

@agorgl i've been using it for two weeks and it works great. I don't know a away of implementing hot reload the way you described.

I set up cider to save file on buffer eval and that's that.

[agorgl]

I I put a lot of thought into the whole reloading / re-evaluation workflow this month.
I came to the conclusion that the whole save something to disk -> auto reload everything + refresh the browser is not the proper clojure way to do things.

It seems that something relevant to this controversy was already discussed on hackernews a couple years back and the linked article along with the comments gives it some useful perspective: https://news.ycombinator.com/item?id=33800175

In this spirit, apart from the modernization that has been done in this PR in the browser integration part, I believe that the reload trigger should be offered as a clojure function / api to be called from the user/dev as an alternative to the filesystem watcher. That way, the user/dev can then decide how to reload/re-eval his code AND trigger a browser refresh from their editor without relying in filesystem changes.

This will not only lead to a more precise repl-driven development experience given the finer-grained control, but will also avoid superfluous reloads/refreshes triggered by code edited and saved in the watched files/folders that isn't relevant to the current browser context.

[licht1stein]

That's how I ended up doing it, actually. But this middleware still works :)

[mikew1]

I'm glad to have found this thread, as I've been using wrap-reload and wrap-refresh with reitit, which seemed to work (did work for a bit, as the app I was building was nascent), however a bit further into development, as the rendering work for one particular page became larger, I began to encounter what are now consistent/reproducible ~40 second pauses on (some) page loads, just for that larger page, not for others.

Sometimes the same page loads instantly, so I initially thought this may be JVM Garbage Collection pause.

However, on a whim I removed wrap-reload and then wrap-refresh, and it turns out the problem is wrap-refresh.
(If I remove only the latter, the pauses disappear).

So when I read above that ring-refresh "didn't work with reitit", that was a light bulb moment.
Do we know if this is still true? (my case seems to suggest this may be so).

Is the way to begin thinking about this in the fact that ring-refresh adds a route (I hadn't thought about detail of how it works until now) and, well, reitit is a router? Hence the possibility of expectations ring-refresh has in place re. the router; not being met?

[licht1stein]

@mikew1 you can try using my fork, it should work.

[sanbornm]

Adding another report of using reitit and wrap-refresh together and experiencing random 30-40s loads on normally fast to load pages. I tracked it down with clj-async-profiler and then found this thread.