Not disposing of disposables on dev reload

[skylize]

Would love to know how to run a test on my claim. Looks to me like extension.cljs#L18 fails to dispose of disposables when deactivating.

(defn- clear-disposables! []
   ;; First we reset disposables to empty vec.
  (swap! db/!app-db assoc :disposables [])
   ;; Then we try to call .dispose on each disposable.
   ;;  But since the vec is empty, this is a no-op.
  (p/run! (fn [^js disposable]
            (.dispose disposable))
          (:disposables @db/!app-db)))

edit: I guess it's not really that hard to test: Just pretend not in VS Code and store some disposables with spy .dispose functions. Still would be nice to know a reasonable way to verify what VS Code is doing here.

[skylize]

The disposables are added to subscriptions, so Code should dispose them for us on deactivate. So I guess this should only be an issue when reloading? Or does reloading reactivate? If so there would be no need to for trying to manually dispose anyway.

[PEZ]

Hmmm, that does look funny. Your idea with using a spy disposable seems reasonable to me. If you register a handler for vscode/workspace.onDidOpenTextDocument which just prints something, you should quickly see if it starts firing more than once.

[skylize]

Create a basic HelloWorld extension with a console log in activate and in deactivate. Define activationEvents": ["*"] in package.json. Open debug, and see "activating" in debug log. Run Developer: Reload Window (Ctrl-R) in the testing window, and see "deactivating" then "activating" in the log.

To verify similar behavior outside dev mode: open Dev Tools Console in the normal window and run Developer: Reload Window. See the console flash clean, and then slowly fill up with various warnings and messages, including such things as "Calva activate START" and "Joyride activate START".

So, we can see reloading includes a deactivate/activate cycle. Code promises to dispose of anything stored in ExtensionContext.subscriptions on deactivate. There should be no reason to concern ourselves with any activation-related disposables, once we have handed them off to subscriptions.

---

Side-note: While going through other less-simple testing ideas, I determined that listing a bunch of Activation Events in package.json is not helpful when you already use the * event (as is currently done in this project). In fact it leads to Code doing extra busy work when those specific events are triggered: You might sometimes see "Activating Extensions" in the Status Bar as it tries but fails to activate the already activated extension.

[PEZ]

There should be no reason to concern ourselves with any activation-related disposables, once we have handed them off to subscriptions

I think we are speaking of different type of reloads. The wiring in this project is so that we can reload the definition of the app without reloading the VS Code window. The reload happens in the REPL. VS Code doesn't even notice it is happening and won't dispose of anything.

[skylize]

Not clear to me when that happens. I guess that is controlled by Shadow's before-load-async hook? What exactly triggers that hook?

Are there some special steps for working with this project in dev mode?

After npm i, I can jack-in with Calva using Jack-In -> \ -> shadow-cljs -> :extension -> F5 (Start Debugging) -> :extension. Seems like that gets me properly connected. For example, I can use vscode/window.showInformationMessage to show a popup in the debug client window. But then I get weird behaviors such as:

(db/extension-context) ;; just expecting some representation of the context in the repl

;; .... 20-30 seconds non-responsive ....

; The previously used runtime disappeared. Will attempt to pick a new one when available but your state might be gone.

[skylize]

BTW, the example scripts have users calling .dispose before clearing their disposable stores, which is what I would have expected to see here as well.

;; from
;;  assets/getting-started-content/workspace/workspace_activate.cljs

(defn- clear-disposables! []
  (run! (fn [disposable]
          (.dispose disposable))
        (:disposables @!db))
  (swap! !db assoc :disposables []))```

[PEZ]

Yes, one instance of the reload in question is when Shadow calls before-load-async. Shadow calls it when it is done compiling files as a result of a file being saved. There is also a rich comment at the bottom of extension.cljs that I use to call this manually (I often update the app without saving files).

Are there some special steps for working with this project in dev mode?

Only the notes in CONTRIBUTE.md. I should make that easier to find and also elaborate a bit more on it. The build script is basically the same as the jack-in you do. The error you get there is due to something that is evaluated when evaluating the extension context, maybe a cyclical reference or something. It's a limitation we have to live with.

BTW, the example scripts have users calling .dispose before clearing their disposable stores, which is what I would have expected to see here as well.

Indeed. I think the bug you found there has gone unnoticed because we seldom update the subscriptions. Please file a PR fixing it, if you have some time for it. Otherwise I can do it, but it is more fun with contributed code, I think. 😄

[skylize]

Is there a reason we are using promesa's p/run! here instead of just run!? In theory there could be asynchronous dispose functions, but as far as I can tell we don't really care either way.

We could extend on this to await disposal before resetting (-> (p/run <<dispose>>) (p/then <<swap!>>), but I don't see a reason to do so.

The example scripts don't bother with promesa. So, if for some reason we do care about possible promises, then the example scripts should probably be altered to reflect this.

[skylize]

If you agree with the assessment that we don't need promesa here, then PR is ready to go.

[skylize]

Weirdly somehow kept getting random stray whitespace changes in the diff. 🤷🏻 But commit looks good now.