Improve error handling (#704)

This improves Clerk's error handling by: * flattening the error hierarchy by not catching and rethrowing eval errors * not having the whole doc on the exception again which doesn't help and looks strange when custom viewers are used * updating the in-memory cache when evaluation fails halfway through a notebook * now throwing during `clerk/show!` when triggered through the file watcher
nextjournal · Oct 8, 2024 · 3835df3 · 3835df3
1 parent ff77e61
commit 3835df3
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 19 deletions.
diff --git a/src/nextjournal/clerk.clj b/src/nextjournal/clerk.clj
@@ -64,18 +64,21 @@
                                         e)))
                       (catch Exception e
                         (throw (ex-info (str "`nextjournal.clerk/show!` could not not parse the file: `" (pr-str file-or-ns) "`")
-                                        {::doc {:file file-or-ns}}
+                                        {:file file-or-ns}
                                         e))))
              _ (reset! !last-file file)
              {:keys [blob->result]} @webserver/!doc
-             {:keys [result time-ms]} (try (eval/time-ms (binding [paths/*build-opts* (webserver/get-build-opts)]
-                                                           (eval/+eval-results blob->result (assoc doc :set-status-fn webserver/set-status!))))
-                                           (catch Exception e
-                                             (throw (ex-info (str "`nextjournal.clerk/show!` encountered an eval error with: `" (pr-str file-or-ns) "`") {::doc (assoc doc :blob->result blob->result)} e))))]
-         (println (str "Clerk evaluated '" file "' in " time-ms "ms."))
-         (webserver/update-doc! result))
+             {:keys [result time-ms]} (eval/time-ms (binding [paths/*build-opts* (webserver/get-build-opts)]
+                                                      (eval/+eval-results blob->result (assoc doc :set-status-fn webserver/set-status!))))]
+         (if (:error result)
+           (println (str "Clerk encountered an error evaluating '" file "' after " time-ms "ms."))
+           (println (str "Clerk evaluated '" file "' in " time-ms "ms.")))
+         (webserver/update-doc! result)
+         (when-let [error (and (not (::skip-throw opts))
+                               (:error result))]
+           (throw error)))
        (catch Exception e
-         (webserver/update-doc! (-> e ex-data ::doc (assoc :error e) (update :ns #(or % (find-ns 'user)))))
+         (webserver/update-doc! (-> @webserver/!doc (assoc :error e) (update :ns #(or % (find-ns 'user)))))
          (throw e))))))
 
 #_(show! "notebooks/exec_status.clj")
@@ -84,7 +87,7 @@
 #_(show! 'nextjournal.clerk.tap)
 #_(show! (do (require 'clojure.inspector) (find-ns 'clojure.inspector)))
 #_(show! "https://raw.githubusercontent.com/nextjournal/clerk-demo/main/notebooks/rule_30.clj")
-#_(show! (java.io.StringReader. ";; # In Memory Notebook 👋\n(+ 41 1)"))
+#_(show! (java.io.StringReader. ";; # In Memory Notebook 👋\n(+ 41 1) (/ 1 0)"))
 
 (defn recompute!
   "Recomputes the currently visible doc, without parsing it."
@@ -137,8 +140,8 @@
             show-file? (or (not @!show-filter-fn)
                            (@!show-filter-fn rel-path))]
         (cond
-          show-file? (nextjournal.clerk/show! rel-path)
-          @!last-file (nextjournal.clerk/show! @!last-file))))))
+          show-file? (show! {::skip-throw true} rel-path)
+          @!last-file (show! {::skip-throw true} @!last-file))))))
 
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

diff --git a/src/nextjournal/clerk/eval.clj b/src/nextjournal/clerk/eval.clj
@@ -232,12 +232,14 @@
         (reduce (fn [state cell]
                   (when (and (parser/code? cell) set-status-fn)
                     (set-status-fn (->eval-status analyzed-doc (count (filter parser/code? (:blocks state))) cell)))
-                  (let [state-with-deref-deps-evaluated (analyzer/hash-deref-deps state cell)
-                        {:as result :nextjournal/keys [blob-id]} (when (parser/code? cell)
-                                                                   (read+eval-cached state-with-deref-deps-evaluated cell))]
-                    (cond-> (update state-with-deref-deps-evaluated :blocks conj (cond-> cell result (assoc :result result)))
-                      blob-id (update :blob-ids conj blob-id)
-                      blob-id (assoc-in [:blob->result blob-id] result))))
+                  (try (let [state-with-deref-deps-evaluated (analyzer/hash-deref-deps state cell)
+                             {:as result :nextjournal/keys [blob-id]} (when (parser/code? cell)
+                                                                        (read+eval-cached state-with-deref-deps-evaluated cell))]
+                         (cond-> (update state-with-deref-deps-evaluated :blocks conj (cond-> cell result (assoc :result result)))
+                           blob-id (update :blob-ids conj blob-id)
+                           blob-id (assoc-in [:blob->result blob-id] result)))
+                       (catch Throwable e
+                         (reduced (assoc state :error e)))))
                 (-> analyzed-doc
                     (assoc :blocks [] :blob-ids #{})
                     (update :->hash (fn [h] (apply dissoc h deref-forms))))

diff --git a/test/nextjournal/clerk/eval_test.clj b/test/nextjournal/clerk/eval_test.clj
@@ -139,8 +139,8 @@
     (eval/eval-string "(ns test-deref-hash (:require [nextjournal.clerk :as clerk])) (defonce !state (atom [(clerk/md \"_hi_\")])) @!state"))
 
   (testing "won't eval forward declarations"
-    (is (thrown? Exception
-                 (eval/eval-string "(ns test-forward-declarations {:nextjournal.clerk/no-cache true})
+    (is (:error
+         (eval/eval-string "(ns test-forward-declarations {:nextjournal.clerk/no-cache true})
 (declare delayed-def)
 (inc delayed-def)
 (def delayed-def 123)")))))