reagent-project / reagent

A minimalistic ClojureScript interface to React.js
http://reagent-project.github.io/
MIT License
4.75k stars 414 forks source link

component-did-update doesn't pass previous state #80

Closed tomconnors closed 9 years ago

tomconnors commented 9 years ago

I'm trying to detect a transition of a component's internal state, set with reagent.core/set-state but only the argv passed when constructing the component is passed to the component-did-update fn. Is there a way to easily detect specific state transitions that only use component-local state? If a means does not currently exist, is it possible to pass the component's previous state into that fn?

Only tangentially related, but worth mentioning: perhaps set-state should be set-state!?

holmsand commented 9 years ago

Good observation – but unfortunately that's quite tricky to do.

Reagent doesn't use React's setState mechanism at all (since that is synchronous, and doesn't play well with ClojureScript data structures). Instead set-state just uses a normal Reagent atom under the hood, in order to get asynchronous rendering.

I'm thinking of exposing that "state atom" – which would allow you to use add-watch to observe changes to it. Would that help in your case?

tomconnors commented 9 years ago

That would solve my problem; I'd just set up the watch in component-will-mount. I'm not familiar with the code enough to know whether this is doable, but maybe reagent can detect that a component-did-update function was provided and set up that watch automatically to call the component-did-update fn.

tomconnors commented 9 years ago

Found a pretty good solution to this that doesn't require any changes to reagent unless you'd like to make the api a bit friendlier. Here's what I did:

;; define a mixin for components that use an internal state atom
;; note that the #js reader macro is required for this to work.
(def component-local-state-mixin
  #js {:componentWillMount
       (fn []
         (this-as this
                  (let [state-atom (reagent.impl.component/state-atom this)]
                    (add-watch state-atom :component-local-state-mixin-watch
                               (fn [k r o n]
                                 (.componentStateDidUpdate this r o n))))))
       :componentWillUnmount
       (fn []
         (this-as this
                  (let [state-atom (reagent.impl.component/state-atom this)]
                    (remove-watch state-atom :component-local-state-mixin-watch))))})

;; use the mixin when constructing a component with reagent.core/create-class.
;; again, #js required.
(def play-view
  (reagent/create-class
   {:mixins #js [component-local-state-mixin]
    :get-initial-state
    (fn [this]
      {:playing? false})
    :component-state-did-update
    (fn [this ref prev-state new-state]
      (if (and (not (:playing? prev-state))
               (:playing? new-state))
        (start-playing!)))
    :render
    (fn [this]
      [:div.play-view])}))

The api-friendliness change would be to put a reference to reagent.impl.component/state-atom in reagent.core.

holmsand commented 9 years ago

In Reagent 0.5.0-alpha3, there is now a reagent.core/state-atom that can be used instead of the one in reagent.impl.component.

tomconnors commented 9 years ago

Cool, thanks a lot!

pandeiro commented 9 years ago

Why is there a need to use a JS object w/ camel-cased lifecycle keys and the :mixins key?

In other words, why would this not work using merge with reagent/create-class to include a regular cljs map with :component-will-mount and :component-will-unmount keys?

tomconnors commented 9 years ago

I don't remember well what the reason was, but I think it's because reagent/create-class performs the vector to array/hashmap to js object conversion shallowly, or the snake-case to camelCase conversion shallowly, or, probably, both of those.

pandeiro commented 9 years ago

Thanks @tomconnors, but still neither of those would seem to prevent it (no need for :mixins at all, so vector/array wouldn't matter, and hyphen-to-camel being shallow also wouldn't matter, since these are at the top level anyway)... Anyway, food for thought.

tomconnors commented 9 years ago

Assuming what I posted above about shallowness is actually the case, the #js reader macro (or the equivalent-ish clj->js fn call) and camel case are needed when using mixins, because componentDidMount and the like are properties of the mixin object, not the class that's created, so they aren't actually top level when reagent sees them. And the functionality I described above did require a mixin, at least as far as I could tell.

pandeiro commented 9 years ago

@tomconnors Cheers, thanks for the explanation. I guess I don't understand what the functional difference between using :mixin and just merging the map is. I'll try to figure that out -- thanks again for your replies!

mike-thompson-day8 commented 9 years ago

It looks to me as if this issue is resolved and can be closed?

tomconnors commented 9 years ago

Yep, using the reference in reagent.core works for me.

mike-thompson-day8 commented 9 years ago

Excellent. Would you mind closing (I don't have perms)?