-
Notifications
You must be signed in to change notification settings - Fork 982
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
Add FlowStorm, a tracing debugger for Clojure(Script) #20054
Conversation
@@ -99,7 +99,8 @@ | |||
"prettier": "^2.8.8", | |||
"process": "0.11.10", | |||
"react-test-renderer": "18.1.0", | |||
"shadow-cljs": "2.26.2" | |||
"shadow-cljs": "2.26.2", | |||
"websocket": "^1.0.35" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: websocket
is a must. Without it, FlowStorm can't run.
1d762b7
to
bc0f58f
Compare
Jenkins Builds
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate the expansion of debugging docs.
This looks promising, I will try to test it during this week. Code-wise I have no objections, I will approve it as soon as I have some time to checkout the branch and test it locally 🚀 |
Much appreciated @briansztamfater 💯 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yesss, thank you!!! Looks great, can't wait to try it out while developing
Got class not found |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ilmotta
I checked it and It seems very powerful, I inspected some variables while sending a transaction since it's a complex part of our code and it looks interesting!
It reminds me to the debugger that IntelliJ has for JVM Clojure, you are able to add breakpoints and inspect the memory at any point.
One problem with my editor is when I add the #trace
line, IntelliJ thinks it's an error so it starts to indent the code weird and mistakenly highlights some vars as errors 😢
Another downside is the UI, It's not clear enough for me but probably it's a matter of getting used to it.
Right now, I'd say I'd prefer the REPL most of the time, but I there are some flows where I need to check what values were passed to a function compared to the current ones, so this debugger will help A LOT! 🎉 .
I'll check what other features it has Later.
It's a good addition, thanks for the PR!
You described exactly how I perceive FlowStorm. As a tool, I missed many times, especially because I used in Clojure JVM more. The more I use it, the more I see its utility.
There's probably a way to make IntelliJ understand zprint also tries to format whatever comes after Many thanks for testing the PR! |
bc0f58f
to
02fa9cf
Compare
cider/cider-nrepl/0.44.0/cider-nrepl-0.44.0.jar | ||
cider/orchard/0.21.0/orchard-0.21.0.jar | ||
cider/piggieback/0.5.2/piggieback-0.5.2.jar | ||
cider/cider-nrepl/0.31.0/cider-nrepl-0.31.0.jar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@siddarthkay, @jakubgs, @yqrashawn: some dependencies keep getting changed because they weren't supposed to be tracked in the file shadow-cljs.edn
. They are not in general in Clojure projects.
The problematic deps are:
cider/cider-nrepl
cider/piggieback
refactor-nrepl
nrepl/nrepl
The reason they change (especially the first 2) is because cider/cider-nrepl
and cider/piggieback
are dependencies for Emacs and we configure specific versions in our machines at ~/.shadow-cljs/config.edn
. Then, when we run nix-update-clojure
, the deps will actually use our local versions, not the project ones. The ones we have in the project's shadow-cljs.edn
are very old and probably useless as well.
Here you see the version going down from 0.44.0
to 0.31.0
because I temporarily renamed the path ~/.shadow-cljs
, so that nix-update-clojure
would pick up the status-mobile
versions instead.
But I doubt I'll remember to do it in the future or that other devs will do the same, and so we keep perpetually changing these lib versions based on local dev needs...
Just removing the deps from shadow-cljs.edn
won't do it because our nix script will still see all dependencies managed by shadow that we need to define outside status-mobile
. One reasonable solution is to filter them out explicitly in the script because it's not like we have a large list of dependencies that keep changing, mostly the ones I shared.
@yqrashawn, do you also customize some of these deps outside status-mobile
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not have these pkgs in my ~/.shadow-cljs/config.edn
Yes, I use a custom babashka script that reads these pkgs version using emacsclient --eval
and I use .envrc.local
to sed
all yarn shadow-cljs
in Makefile
to the bb script
~/local/bin/sstart server
> npx shadow-cljs -d cider/cider-nrepl:0.47.0 -d nrepl/nrepl:1.1.1 -d refactor-nrepl/refactor-nrepl:3.10.0 server
sed -i 's/yarn shadow-cljs/~\/local\/bin\/sstart/g' Makefile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @yqrashawn. So it's good to see you also do something about these dependencies because it probably means a quick fix is valuable/worth our time since we don't want every Emacs user to have to figure this sort of thing by themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll create an issue in any case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ilmotta
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #20324
Summary
This PR adds FlowStorm
v3.7.5
, a well known (tracing) debugger for Clojure(Script). The kinds of things you can do with it are beyond my ability and time to explain in this humble PR description. SeeDemo
section for a real example in our codebase, but we are all encouraged to check out many other features (seeResources
section below).Demo
I recorded a simple example (please, watch in fullscreen) showing you how I can debug the event handler to
login
. After that, you see me stepping back and forth in all available expressions. The output of each expression or symbol is displayed on the right panel. At the end, you also see me defining a var from within the debugger, which is then available in myuser
namespace and which can be manipulated and inspected as if it was defined right from within my text editor (REPL is connected of course). This is very useful when analyzing larger/more data structures, since you are no longer dealing with raw print statements.demo-flow-storm.webm
To summarize, with this tool, you can debug almost any cljs function in
status-mobile
. FlowStorm in CLJS is not as capable as on the JVM, but its main features work well enough.How do I use it?
Please, check the markdown diff in this PR:
doc/debugging.md
.When would you use FlowStorm in
status-mobile
?You can use it all the time if you want, but FlowStorm can be a powerful tool to understand complex pieces of code. Consider those large subscriptions or event handlers. Or also all those components full of calculations. Understanding some of these things is no easy task, even with a REPL. It is not a replacement for re-frisk, those are very different tools and each have their place.
Resources
Platforms
Areas that may be impacted
None.
Steps to test
The tool should work on iOS, but I kindly ask a developer with iOS to double-check. The e2e test suite is more than enough to guarantee this PR won't cause regressions.
status: ready