day8 / re-com

A ClojureScript library of reusable components for Reagent
https://re-com.day8.com.au
MIT License
797 stars 147 forks source link

Typeahead doesn't render selection when ratom deref in parent #138

Closed colindresj closed 7 years ago

colindresj commented 7 years ago

Typeahead doesn't render first selection if there happens to be a dereferenced ratom in the parent component. Here's a stripped-back example of where I encountered this:

(defn- data-filter-fn [value]
  (let [suggestions ["my" "suggestions" "here"]
        pattern (re-pattern (str "(?i)" value))]
    (vec (filter #(re-find pattern %)) suggestions))))

(defn add-person []
  (let [submittable? (reagent/atom false)]
    (fn []
       @submittable?
       [re-com/typehead
         :data-source data-filter-fn
         :rigid? true
         :change-on-blur? true
         :on-change #(reset! submittable true)])))

With that code, I'm able to select from my suggestions, but the first time I do so, it's not appearing inside the text input. Every time thereafter works fine. If I remove the deref on submittable?, I don't have a problem anytime, including the first time.

I've found that I can deref ratoms without encountering a problem, as long as they're not changed in the on-change callback.

Gregg8 commented 7 years ago

@blak3mill3r, would you have time to look at this please?

blak3mill3r commented 7 years ago

I just noticed this, yes I will have a look.

Gregg8 commented 7 years ago

Thanks. Would also appreciate your thoughts on #130 when you get a chance.

blak3mill3r commented 7 years ago

@colindresj

I'm unable to reproduce this problem. Here's what I tried:

I had to modify your code a little to get it to compile. cljs.core/vec takes only 1 argument, so I changed it to (into [] ... ). The parens aren't balanced in data-filter-fn, and you didn't include how you're using the component add-person.

The code below works, and in my test it didn't exhibit the problem you described.

I hope that helps, I apologize for not noticing this sooner.

One possible reason it might not work the way you expect is you're dereferencing the ratom in the body of the component function, but not in the component it returns (the [:div]). I'm not sure exactly how that will interact with the ratom deref tracking in reagent, but it doesn't seem like a useful thing to do (why deref the atom if it doesn't affect the rendering of the component?).

(defn- data-filter-fn [value]
  (let [suggestions ["my" "suggestions" "here"]
        pattern (re-pattern (str "(?i)" value))]
    (into []
          (filter #(re-find pattern %))
          suggestions)))

(defn add-person []
  (let [submittable? (reagent/atom false)]
    (fn []
      [:div
       [:pre (prn-str @submittable?)]
       [typeahead
        :data-source data-filter-fn
        :rigid? true
        :change-on-blur? true
        :on-change #(reset! submittable? true)]])))

(defn page-test []
  [:div
   [:h1 "typeahead test:"]
   [add-person]])
colindresj commented 7 years ago

@blak3mill3r sorry, took me a while to get back to you.

I rushed when I wrote the example (in the github editor no less), so had some typos. The filter function's supposed to look like this:

(defn- data-filter-fn [value]
  (let [suggestions ["my" "suggestions" "here"]
        pattern (re-pattern (str "(?i)" value))]
    (vec (filter #(re-find pattern %) suggestions))))

The example I wrote is definitely contrived, so doesn't make much sense. I've pasted below a more real example:

(rf/reg-event-db
  ::on-change
  rf/trim-v
  (fn [db [person]]
    (assoc db ::person-selector person)))

(rf/reg-sub
  ::selected-person
  (fn [db _]
    (::person-selector db)))

(defn- person-full-name [person]
  (str (:person/first-name person)" " (:person/last-name person)))

(defn- person-has-any-role? [person role-set]
  (not-empty (clojure.set/intersection role-set (set (:person/roles person)))))

(defn- person-selector-data-source
  [everybody
   {:keys [suggestion-size roles predicate]
    :or   {suggestion-size 10}}
   text]
  (let [pattern (re-pattern (str "(?i)" text))]
    (->> everybody
         (filter (fn [p] (or (nil? roles) (person-has-any-role? p roles))))
         (filter (fn [p] (or (nil? predicate) (predicate p))))
         (filter (fn [p] (re-find pattern (person-full-name p))))
         (into [])
         (take suggestion-size)
         (sort-by person-full-name))))

(defn- person-selector-render-suggestion [suggestion]
  [:span (person-full-name suggestion)])

(defn person-selector-typeahead []
  (let [people   (subscribe [:person/all])
        selected (subscribe [::selected-person])]
    (fn [{:keys [on-add] :as opts}]
      [:div      
        [rc/typeahead
         :model                selected
         :data-source          (partial person-selector-data-source @people opts)
         :render-suggestion    person-selector-render-suggestion
         :suggestion-to-string person-full-name
         :placeholder          "Select a person..."
         :rigid?               true
         :on-change            #(rf/dispatch [::on-change %])]
       [:button
        {:on-click #(on-add @selected)}
        "Add Team Member"]])))

;; Calling component
(defn add-team-member []
  (fn []
    [:div
     [person-selector-typeahead
      {:roles #{:admin}}]])

;; Contrived :person/all sub
(rf/reg-sub
  :person/all
  (fn [_ _]
    [{:person/first-name "Tony"
      :person/last-name  "Soprano"
      :person/roles      #{:admin}}
     {:person/first-name "Carmela"
      :person/last-name  "Soprano"
      :person/roles      #{:admin}}
     {:person/first-name "Anthony Jr."
      :person/last-name  "Soprano"
      :person/roles      #{:user}}]))
blak3mill3r commented 7 years ago

@colindresj

Okay, since I cannot run that snippet as-is (what is rf?)... I'll do my best to offer something anyway, I do spot one thing that might be a snag and is almost certainly not what you intended:

When you use clojure.core/partial it creates an instance of IFn with the partially-applied arguments baked in as values, so that means when you deref the people atom above (which happens whenever person-selector-typeahead is rendered) it takes a snapshot of the value in that atom due to the use of partial. That means, IIUC, that when the typeahead calls your datasource person-selector-data-source the value of the everybody argument is going to be this snapshot of the value of the atom as of the last time person-selector-typeahead was rendered. This highly non-obvious side-effect of rendering is probably going to be a source of confusion.

I hope that explanation makes sense. I think if you add a (println everyone) to your data-source fn, you'll see what I mean.

colindresj commented 7 years ago

Oh right, thanks @blak3mill3r. I make that mistake all the time. Ps - rf in this example was re-frame.