Catch not= throwing exceptions and log a warning

[danielcompton]

Under certain conditions, not= in the default shouldComponentUpdate function can throw an exception.

You can boil this down to:

cljs.user=> (not= {:todos 1} (sorted-map 1 2))
Cannot compare :todos to 1
	cljs.core/-compare [cljs.core/IComparable] (cljs/core.cljs:9995:14)
	-compare (cljs/core.cljs:807:24)
	compare (cljs/core.cljs:2360:5)
	entry-at (cljs/core.cljs:8401:25)
	cljs.core/-lookup [cljs.core/ILookup] (cljs/core.cljs:8468:24)
	cljs.core/-equiv [cljs.core/IEquiv] (cljs/core.cljs:6560:32)
	-equiv (cljs/core.cljs:695:23)
	= (cljs/core.cljs:1247:16)

This is ultimately a bug in ClojureScript (and Clojure), but in the general case, not= doesn't guarantee that it won't throw, and there are weird edge cases like this that pop up now and then when comparing composite data types (especially when they are not standard vector/map/set/list's).

You could imagine try/catching and swallowing the exception, which is technically correct as you can make a good assumption that if a comparison failed then the two objects aren't equal. But this feels pretty gross.

I propose that there is a try/catch which prints a warning/error explaining what happened, and that you can fix it by synthetically setting the key to be different for each piece of data you provide as an argument to the component.

Todo:

• Finish wording in error message
• Add a docs page explaining in more detail how to address this issue
• Test the performance impact of try/catch in this position
• Add tests covering this error

[atroche]

@danielcompton I had a stab at writing a test for this. It gives me an appropriate failure message on master (and passes with your changes), although in the node test environment it passes either way.

Also I can't figure out why I need to override window.onerror to stop an uncaught exception on r/flush (see big comment below) in Headless Chrome, when I don't need to in my experiments with Chrome + Figwheel.

(deftest on-failed-prop-comparison-in-should-update-swallow-exception-and-do-not-update-component
  (let [prop (r/atom {:todos 1})
        component-was-updated (atom false)
        error-thrown-after-updating-props (atom false)
        component-class (r/create-class {:reagent-render (fn [& args]
                                                           [:div (str (first args))])
                                         :component-did-update (fn [& args]
                                                                 (reset! component-was-updated true))})
        component (fn []
                    [component-class @prop])
        check (fn []
                (reset! prop (sorted-map 1 2))

                ;; Not clear on why I have to override window.onerror, I would
                ;; expect the try/catch to be enough. But if I remove it, I get
                ;; an uncaught exception error.
                (let [wrapped-flush (wrap-capture-window-error (fn [] (r/flush)))]
                  (try
                    (wrapped-flush)
                    (catch :default e
                      (reset! error-thrown-after-updating-props true))))

                (is (not @component-was-updated))
                (is (not @error-thrown-after-updating-props)))]

      (when isClient
        (with-mounted-component [component]
                                check))))

[atroche]

Oh, and that test is for https://github.com/reagent-project/reagent/blob/master/test/reagenttest/testreagent.cljs. It uses some utility fns in there and follows roughly the same pattern as the test named lifecycle.

[atroche]

I also ran this little performance test a few times in my browser and noticed no difference between master and your branch, @danielcompton:

https://gist.github.com/atroche/f33614a0976cc6181bee30ad9a88be9c

(And I checked that it's hitting the relevant not= expression.)

[Deraen]

@danielcompton @atroche Thanks for this. I merged this now with a bit modified test case.

I think the error message is quite okay, the original error message (e.g. Cannot compare :todos to 1) is quite informative.