day8 / re-frame

A ClojureScript framework for building user interfaces, leveraging React
http://day8.github.io/re-frame/
MIT License
5.4k stars 715 forks source link

[Enhancement]: Subscriptions that return `nil` or `false` are evaluated regardless of what's in cache #777

Closed p-himik closed 10 months ago

p-himik commented 1 year ago

What do you suggest?

This is how the cache lookup is implemented:

(defn cache-lookup
  ([query-v]
   (cache-lookup query-v []))
  ([query-v dyn-v]
   (get @query->reaction [query-v dyn-v])))

And this is the important part of how it's used:

(if-let [cached (cache-lookup query dynv)]
  cached
  (... compute the sub, cache, and return the result ...))

if-let returns its "else" branch when cached is either nil or false. And thus it always recomputes the sub value when it's false. if-some would be better because then it would return the "else" branch only when the value is nil. But a proper solution would use some singleton value with that get from the first block and check for it instead of using if-let/if-some.

kimo-k commented 10 months ago

Hey @p-himik, do you have a repro for this?

The vals of the cache are reactions. I'm not sure what a user would do to put a nil or false in there.

p-himik commented 10 months ago

You're absolutely right, this issue is just a documentation of my poor judgement on that day.

kimo-k commented 10 months ago

Yeah it is kind of vexing when re-frame double-derefs things to get the job done, haha.