Skip to content

Commit

Permalink
fix <-effect with Clojure data
Browse files Browse the repository at this point in the history
  • Loading branch information
lilactown committed Mar 17, 2019
1 parent 510c845 commit ba0d520
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 2 deletions.
16 changes: 15 additions & 1 deletion src/hx/hooks.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,21 @@
([f]
(react/useEffect f))
([f deps]
(react/useEffect f (to-array deps))))
;; React uses JS equality to check of the current deps are different than
;; previous deps values. This means that CLJS data (e.g. maps, sets, vecs)
;; equality is not respected and will trigger if you e.g. pass in a vec of
;; strings as props and need to depend on that inside of an effect.
;;
;; We can work around this by assigning the previous deps to a ref, and do
;; our own equality check to see if they have changed. If so, we update the
;; ref to equal the current value.
;;
;; We can then just pass this one value into `useEffect` and it will only
;; change if Clojure's equality detects a difference.
(let [-deps (react/useRef deps)]
(when (not= deps (.-current -deps))
(set! (.-current -deps) deps))

This comment has been minimized.

Copy link
@roman01la

roman01la Mar 19, 2019

Just wanted to point out that in React's docs they don't recommend setting ref during render function call because it can lead to unexpected behavior.

Conceptually, you can think of refs as similar to instance variables in a class. Unless you’re doing lazy initialization, avoid setting refs during rendering — this can lead to surprising behavior. Instead, typically you want to modify refs in event handlers and effects.

https://reactjs.org/docs/hooks-faq.html#is-there-something-like-instance-variables

This comment has been minimized.

Copy link
@lilactown

lilactown Mar 19, 2019

Author Collaborator

I think this is fine since we both set and read the ref synchronously. I believe if we were using the ref e.g. inside the useEffect, then it would potentially be surprising since React could have called our render function multiple times before the useEffect is triggered.

This comment has been minimized.

Copy link
@roman01la

roman01la Mar 19, 2019

Agree. I've come to the same conclusion. I'm still a bit worried about what could possibly go wrong, but there's not much info about that.

(react/useEffect f #js [(.-current -deps)]))))

(def <-context
"Just react/useContext"
Expand Down
2 changes: 1 addition & 1 deletion test/hx/hooks_test.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@
(-> (hx/f [ValTest {:some-val {:asdf "jkl"}}])
(re-render))
(t/is (u/node= (u/html "<div>1</div>")
(u/pret (u/root val-test))) "map")))
(u/root val-test)) "map")))

(t/deftest <-effects-deps-vec
(let [val-test (-> (hx/f [ValTest {:some-val [:asdf :jkl]}])
Expand Down

0 comments on commit ba0d520

Please sign in to comment.