flow-storm / flow-storm-debugger

A debugger for Clojure and ClojureScript with some unique features.
The Unlicense
708 stars 32 forks source link

Browser tab changes not applied in real-time #171

Closed velios closed 4 months ago

velios commented 7 months ago

Hello. First of all thx for amazing tool and that you find time to develop the project so dynamically.

Recently browser tab in ClojureStorm mode was improved. But the changes in this tab are not applied and I would like to know if this is intended. Let me give an example.

Let's say I created a simple pedestal application using the official guide. lein new pedestal-service the-next-big-server-side-thing

Now I launch the application with the storm enabled.

:dependencies
[[com.github.flow-storm/clojure "1.12.0-alpha11"]
 [com.github.flow-storm/flow-storm-dbg "3.15.3"]]

:jvm-opts ["-Dclojure.storm.instrumentAutoPrefixes=true"]

Now i follow instructions and run app with (def dev-serv (run-dev)) and visit example route localhost:8080 and see Hello world message. All fine.

My browser tab looks like this

image

Now I realized that I want to see the interceptors that work when the handler is called. I know I'm interested in the function io.pedestal.interceptor.chain/try-f, so i add ns io.pedestal.interceptor.chain in browser tab

image

And now my browser tab looks like this

image

But if i visit example route localhost:8080 again I still don't see the function try-f. Actually no one function from added io.pedestal.interceptor.chain ns. I tried pressing all the refresh buttons I saw, but nothing changed.

But if before the start i add configuration "-Dclojure.storm.instrumentOnlyPrefixes=io.pedestal.interceptor.chain" and compeletly refresh project. All works as expected and try-f captured by flowstorm.

The same is true for skip ns and skip regex functionality.

image
jpmonettas commented 7 months ago

Hi @velios. Glad you find FlowStorm useful!

Yes this is not a bug, but how things work. When you start the JVM with "-Dclojure.storm.instrumentOnlyPrefixes=io.pedestal.interceptor.chain" then all functions in namespaces with that prefix will be compiled instrumented, so you can record them.

When you start withtout the prefix, then all those functions will be compiled normally (like with the official compiler) and just adding a prefix with the browser doesn't change anything, just tells the compiler that whatever gets compiled from now on under those namespaces should be instrumented.

What you need after adding the prefix is to reload those namespaces. Take a look at the note here on the User's Guide https://flow-storm.github.io/flow-storm-debugger/user_guide.html#_modifying_instrumentation_with_the_browser for various ways to do it.

velios commented 7 months ago

Thx for answer, @jpmonettas

I've read the documentation, but there are still questions that I have. Using the same example that I started with above.

The general list of namespaces lists all the namespaces that I loaded into the project, and not just those that I asked to instrument. And it really gives me the ability to follow the documentation and reload the namespaces that I want to track.

image

But at the same time it gives a feeling of some inconsistency. Because Add instrumentation prefix works with patterns, but Reload namespace with specific namespace. So i need to click every namespace and reload it, but i can't reload it by pattern. Perhaps my proposal is incorrect and I simply could not understand why the behavior is the way it is now, but I would like to suggest the following.

image

If this is due to performance issues, then for those who are not concerned about this, it is possible to make a configuration key so that there is no dialog window every time. Like this. -Dclojure.storm.autoReloadInstrumentedNamespaces=true

This seems to be a different proposal, but functionality like on next screenshot also feels valuable.

image

I drew examples only for the add pattern, but the same capabilities seem to be in demand for delete patterns. And it seems like it would provide useful visual feedback to better control what will be recorded next time.

jpmonettas commented 7 months ago

I think your proposal is great, I have also considered it. I just haven't implemented it yet because namespaces needs to be loaded in topological order, so I'll have to include or replicate some of the code in tools.namespace or clj-reload. In the meantime you can also do a (require 'some.lib.core.ns :reload-all) That should recursively reload all de dependents namespace in order.

jpmonettas commented 7 months ago

I just implemented this to use clj-reload when it is on the classpath but just figured that clj-reload can currently only reload namespaces from files coming from your sources and not from libraries. I'll check first how hard it is to implement that on clj-reload and maybe suggest it as a feature.

My idea was to show an alert after changing a prefix saying that FlowStorm can automatically reload the namespaces for you if clj-reload was on the classpath. This way we don't need to maintain code for topologically re-loading namespaces (which also need ns dependencies parsing) and reloading inside FS codebase.

velios commented 4 months ago

I am sorry to bother you. I saw you proposed the first version to a third-party library, I wanted to know if a second attempt is planned. A very desirable feature.

jpmonettas commented 4 months ago

@velios hey, it doesn't bother me. Yeah I'm planning to address that after I finish some other stuff I'm working on. Still not sure if I'm going to work on adding it to clj-reload (which looks like it is going to be a lot of back and forth) or just implement that functionality directly in FS.

velios commented 4 months ago

Yes, I saw in your correspondence that you have already generally implemented all the necessary parts. It seems that any option that takes less of your time will be great. I'm looking forward to trying out the new features you're working on. I literally can’t get through a single day without using flowstorm. Today I’m dissecting the interceptors in the pedestal and I can’t imagine how I would figure it out without a flowstorm.

jpmonettas commented 4 months ago

Ok, this is on master now. You can also now add instrumentation prefixes from the browser like this :

reload-menu

and after changing any prefix you should get a confirmation like this :

reload-menu-confirmation

Accepting it should reload all namespaces affected by the prefix in topological order

velios commented 4 months ago

@jpmonettas Hooray! Thank you. Holiday on my street. Stupid question, but how to use dependency without release?

jpmonettas commented 4 months ago

If you want to try this you can use :

com.github.flow-storm/flow-storm-debugger {:git/sha "3855f977aa34ec8284d2e518856b3b32cfbfe6e2"}

Or you can release and install it locally if you clone flow-storm sources and then run :

VERSION=3.17.0-SNAPSHOT make install-dbg

and then use :

'com.github.flow-storm/flow-storm-dbg {:mvn/version "3.17.0-SNAPSHOT"}`

The git sha is better because you can try different commits pretty easily

velios commented 4 months ago

Thx for mention two options for doing that. git sha is obviously better, but install is still good option for leiningen based projects.

jpmonettas commented 4 months ago

Yeah, true that is better for lein. Keep in mind that make install-dbg will run build.clj. It will check https://github.com/flow-storm/flow-storm-debugger/blob/master/build.clj#L43-L48 your JVM version, and stop building if you are building with JVM >=21. I added this so I don't AOT compile and deploy to Clojars something built with JVM 21, because then it doesn't work with let's say JVM 17. But if you are using 21, just delete the body of that check-jvm function before the make install-dbg or it will not let you build.

velios commented 4 months ago

Yes, I encountered this problem. Fortunately i use asdf cli util which have ability to solve problems like this. Without change anything in code.

Automatic namespace reload did not work for me when i try it in quick test. I'll try to read the documentation to feature, try again and than write result here.

jpmonettas commented 4 months ago

Remember that reloading/recompiling not always does what you expect in Clojure when state is involved. The classic one being :

(defn handler [req] ...)
(def server (jetty/run-server handler {})

No matter how many times you re-evaluate hander the jetty server will keep calling the old function. But please re-open this or create a new issue if you see reloading failing when it shouldn't.