Improve exception handling associated with subs

[olivergeorge]

This code will throw errors due to the registered sub handler throwing an exception. The exception gets cached so that the same error appears after fixing and re-registering the sub at the repl.

In order to repeat remove the assert or register a new sub which doesn't throw then re-render the app.

The workaround is calling clear-subscription-cache!.

Ideas to improve this behaviour and make it less surprising

• clear the cache when subs are re-registered to avoid data getting out of sync
• don't cache exceptions

(ns hello-world.case1
  (:require [re-frame.core :as rf]
            [reagent.dom :as rdom]))

(rf/reg-sub ::throwing-sub (fn [_ _] (assert (= 2 4)) "fn sub"))

(defn app []
  [:p @(rf/subscribe [::throwing-sub])])

(rdom/render [app] (js/document.getElementById "app"))

[mike-thompson-day8]

@olivergeorge In your reload function, are you calling re-frame.core/clear-subscription-cache!, like this: https://github.com/day8/re-frame/blob/master/examples/todomvc/src/todomvc/core.cljs#L58

[olivergeorge]

This repro was done using vanilla clojurescript REPL (as per the getting started guide).

I think it's good to consider the REPL experience rather than relying on hooks associated with hot-reloading. From the REPL I can call clear-subscription-cache! manually - but it's another thing to remember.

[olivergeorge]

Here's a concrete suggestion. What if the cache invalidated when a subscription was re-registered.

The cache should definitely not be trusted in this case since the subscriptions would be out of sync.

The old "warning: re-registering handler" message is gone but how about in it's place we invalidate the cache when it's a subscription re-register.

If this was implemented there would be one less "setup step" for effective re-frame development and the REPL experience would be more seamless.

[olivergeorge]

Thinking about possible complexities / side effects...

Clearing the subscription cache doesn't appear to trigger a re-render of the app. We can clear the cache without other code running (good) but that seems to imply we'd still need a hot-reload hook to tell the app to refresh itself.

[olivergeorge]

Here's a possible approach which modifies reg-sub https://github.com/day8/re-frame/commit/57959c21d0ec00bab71bcb23fd5a3fd32e280223