omcljs / om

ClojureScript interface to Facebook's React
6.66k stars 362 forks source link

a version of om.core/set-state! w/o re-render #134

Closed swannodette closed 9 years ago

swannodette commented 10 years ago

If we provide this does it make sense to provide a version of om.core/transact! that doesn't trigger a re-render?

danielsz commented 10 years ago

Good (and difficult) API design question. You probably don't want duplicate versions of the API, with and without render. What I can say is that when I'm reaching for set-state!, I never want a re-render. Which is why I never put it to use. And I don't really understand the use case. Maybe transact! should always trigger a render, and set-state!never. Simple solution. But would it be encompassing too?

swannodette commented 10 years ago

@danielsz look at the sortable example in the repo, I would not want to pollute the application state with dragging state.om.core/set-state! is going to continue working as it does today for the foreseeable future.

danielsz commented 10 years ago

@swannodette Will look at it. Thanks.

selfsame commented 10 years ago

I've been using an atomic map to store per-component data without triggering renders as part of a sugar wrapper. It's very useful for things like drag cycles where I want to cache values without render cascades. source, sortable example.

It also solves problems where I want to determine what has changed during the IWillUpdate cycle, cache the answer, and retrieve it during render to use as a conditional for passing :state map to descendants.

For the core, I don't have a preference between a renderless set-state! or a third data type, but I do see a lot of utility from the concept. I can't think of a situation to not render a transact! though.

martinklepsch commented 10 years ago

Could you give an example for when a “silent” set-state is useful? I don't quite understand why you'd want to do this at all.

If you want to have state that does not affect the DOM then that's just a matter of not using that state in render-state. The re-render that is being triggered is not very expensive I'd assume and you could later even remove the re-render by modifying should-update.

selfsame commented 10 years ago

@mklappstuhl I agree that react 'renders' are very fast if they don't actually change the html string, but in practice I've still managed to muck up my FPS by triggering them too often.

My specific example is draggable UI, to optimize I ended up silently caching the initial/previous mouse position on the component, and altering the dom node style directly. screenshot

In general, any time I have component state that I know doesn't affect the html of the component or it's children, I'd rather store it 'silently' than trigger render code on the target, ancestors, and any components that share a cursor path with the aforementioned.

ddellacosta commented 10 years ago

I would like to argue for this feature. If state means local component state, specifically anything like position of DOM elements or flags to update certain behaviors, then there are many cases where having a non-render-provoking state update function would be very useful--and in fact the correct behavior.

I may, for example, want to set a flag for an action which I want to unset once the action has been triggered--it may be triggered from a user's behavior or other system events--but I don't want to re-render necessarily just because I've "flipped the switch" back on a flag.

optevo commented 10 years ago

+1 This should significantly simplify keeping local state synchronized with contenteditable divs in the issue I raised yesterday. The forced re-rendering when state is updated is what forces the component to track the position of the caret before an update then reposition it correctly afterwards. I think this is ugly it means having to reimplement standard caret behavior which is already present in the browser.

swannodette commented 10 years ago

@optevo while I sympathize it seems to me that your issue is something React devs must encounter. So I until I hear more about that, I don't think I find this a compelling reason for this feature at this point in time.

swannodette commented 10 years ago

@ddellacosta your use case is too vague. Do you have a simple case a where re-rendering is a performance problem or detectable by the user in some way?

optevo commented 10 years ago

@swannodette there is a React issue for this:

https://github.com/facebook/react/issues/278

I infer from the last comment that handling contenteditable state in React is possible to more cleanly as you can avoid having the component rendered each time.

On 3 April 2014 23:06, David Nolen notifications@github.com wrote:

@optevo https://github.com/optevo while I sympathize it seems to me that your issue is something React devs must encounter. So I until I hear more about that, I don't think I find this a compelling reason for this feature at this point in time.

— Reply to this email directly or view it on GitHubhttps://github.com/swannodette/om/issues/134#issuecomment-39443073 .

swannodette commented 10 years ago

@optevo ok, if React cannot yet solve the contentEditable problem we aren't going to either. Anyways, contentEditable is not a good reason for moving on this ticket. Thanks.

aroneous commented 10 years ago

This might well be a lack of understanding on my part, but doesn't a full re-render on 'set-state!' effectively blow away the state you were trying to set? That seems to be the behavior I'm seeing in at least some cases. E.g., for the 'typeahead' example (which uses local state in the way I have in mind), if you wrap the 'typeahead' component in a 'div' rather than having it at the root level, you get some wonky behavior:

(om/root (fn [app owner]
           (dom/div nil (om/build typeahead app)))
         app-state {:target (.getElementById js/document "app")})

The first time you type a letter into the text box, a full re-render is triggered, wiping out the state. The typed letter doesn't show and no filtering is performed. After the first try, entering text 'works' - based on some poking around, the component is being updated but not re-rendered (as happens in React when you do a setState). For a similarly-simple component that I'm working on the component is always re-created on 'set-state!', rather than just the first time. I have yet to understand that difference in behavior.

Suffice to say, something seems not-quite-right with the current response to 'set-state!' (or it's user error?).

swannodette commented 10 years ago

@aroneous it is an error to return anything other than an IRender or IRenderState instance from a component function. This is documented.

aroneous commented 10 years ago

@swannodette Sorry; what are you referring to as the 'component function' in this case? typeahead does return an IRenderState instance. If you're referencing the anonymous function passed to om/root (which returns a dom/div), your statement would seem to contradict the first example in the Basic tutorial:

(om/root
  (fn [app owner]
    (dom/h2 nil (:text app)))
  app-state
  {:target (. js/document (getElementById "app"))})

This function [i.e., the first parameter to om/root] must return an Om component, a React component, or some other value that React itself knows how to render.

swannodette commented 10 years ago

@aroneous the tutorial is just out of date. Anyone can update them.

aroneous commented 10 years ago

@swannodette At the risk of being a PITA, I'll note that wrapping the return value in an IRender instance didn't seem to make a difference in the behavior:

(om/root (fn [app owner]
           (reify
             IRender
             (fn []
               (dom/div nil (om/build typeahead app)))))
         app-state {:target (.getElementById js/document "app")})

FWIW, the documentation for om/root is consistent with what the tutorial says:

f is a function returning an instance of IRender, IRenderState, a React component, or some value that React knows how to render.

swannodette commented 10 years ago

@aroneous I've fixed the wiki documentation. I'm still not clear on your earlier points. Please point me to a repository that shows the minimal code that demonstrates the problem and open a new GitHub issue linking to it. This discussion isn't relevant for this particular issue. Thanks!

aroneous commented 10 years ago

Apologies; I had a silly typo in my latest attempt. The problem I was seeing doesn't happen when returning a proper IRender instance from the component function. Cheers!

tony-landis commented 10 years ago

I have a use case where being able to do a non-rendering state change would increase performance. I have a drop down that can be triggered either by onClick or onFocus. A click event will trigger both events at almost the same time, causing the drop down to appear and then instantly disappear.

As a workaround, I set-state! :click-delay true on the first event that comes in, and after 150ms set it back it to false, which triggers a unneeded render.

(defn focus-in  [app owner] 
  (om/set-state! owner :opened true)
  (om/set-state! owner :click-delay true)
  ; onFocus happens before onClick, so we have to
  ; ignore onClick for 0.2 ms, in case
  (go (<! (timeout 150))
      (om/set-state! owner :click-delay false)))

(defn click-on [app owner] 
  (when-not (om/get-state owner :click-delay) 
    ;click-delay has expired, enable state change
    (om/set-state! owner :opened 
                   (if (om/get-state owner :opened) false true))))

Instead of increasing the API surface area by adding a silent-state! function, I propose placing metadata on keys in the init-state map with a list of keys that should never trigger rendering, or alternatively, any key with the :silent/ ns.

This means that the silent update could be defined by the creator of a component and the implementor would always be able to call set-state!.

I don't think it is a huge issue if this feature does not get implemented in some fashion. But having the ability to scope parts of local state which is ignored by the re-rendering triggers would be useful for cases like these.

Another option would be have a protocol like IWillUpdate which returns true or false. If not true, re-rendering will not take place on the current diff.

pleasetrythisathome commented 10 years ago

I'm strongly opposed to state changes not triggering re-render by default. IShouldUpdate is already available exactly for this purpose and is exactly the protocol you're proposing. In the docs IShouldUpdate is presented with a disclaimer, and it well should be, but if there is a specific use case like the ones any of you might be describing, you can just write IShouldUpdate to return false when the change in local state should not trigger re-render.

This is react convention as well, and would be a radical departure from the React/Om mentality that component state is always the result of a pure function (IRender or IRenderState) that take component props (cursors) and local state as input. If you can change state without a re-render (that you didn't explicitly prevent in IShouldUpdate), you can no longer safely reason about the state of the DOM.

the-kenny commented 10 years ago

@tony-landis how expensive is this unnecessary re-render in your case? It doesn't sound like it's problematic at all.

selfsame commented 10 years ago

Another use would be a debugging component that counts/displays the number of renders.

@tony-landis perhaps you can roll your own non-rendering component data map, I use this with om 0.5.3:

(def PRIVATE (atom {}))

(defn owner-key [owner]
  (let [cursor (.-__om_cursor (.-props owner))
        path (or (.-path cursor) "?")
        k (or (.-key (.-props owner)) "?")]
    (apply str (conj path k))))

(defn private! [owner korks f]
  (let [func (if (= (type #()) (type f)) f (fn [v] f))
        kcol (if (sequential? korks) korks [korks])
        okey (owner-key owner)]
    (swap! PRIVATE update-in (cons okey kcol) func )))

(defn private
  ([owner]
   (private owner []))
  ([owner korks]
  (let [kcol (if (sequential? korks) korks [korks])
        okey (owner-key owner)]
    (get-in @PRIVATE (cons okey kcol)))))
pleasetrythisathome commented 10 years ago

or just write IShouldUpdate to exclude the keys you don't care about

(defn component [cursor owner opts]
  (reify
    om/IInitState
    (init-state [_]
      {:matters true
       :ignore-me "changing me doesn't cause re-render"})
    om/IShouldUpdate
    (should-update [this next-props next-state]
      (let [prev-props (om/get-props owner)
            prev-state (om/get-render-state owner)
            ignore-keys [:ignore-me]]
        (or (not= prev-props next-props) (apply not= (map #(apply dissoc % ignore-keys) [prev-state next-state])))))
    om/IRenderState
    (render-state [this state]
      ; render something
      )))
danielytics commented 10 years ago

EDIT:

I see that pleasetrythisathome's suggestion actually solves this. Maybe a helper function or documentation entry is all that's really needed.

I guess said helper function could look like this:

(defn dont-update-for-keys [ignore-keys next-props next-state]
  (let [prev-props (om/get-props owner)
        prev-state (om/get-render-state owner)]
    (or (not= prev-props next-props) (apply not= (map #(apply dissoc % ignore-keys) [prev-state next-state])))))

And used like this:

    om/IShouldUpdate
    (should-update [this next-props next-state]
      (dont-update-for-keys [:ignore-me] next-props next-state))

Original comment:

:+1: I recently needed something like this because going through component local state was too slow. I see that in the Goya source, a similar issue is encountered. I handled it by wrapping my component in a function which provides an atom that can be updated without triggering rerenders and it was much faster:

(defn make-my-component []
  (let [invisible-state (atom {})]
    (fn [props owner opts]
      (reify
        ...

Not sure how safe this is if called directly in render: (om/build (make-my-component) app-state) - this would create a new copy of invisible-state each render, right? (I didn't do it like this - I stored it globally.. YUCK)

tony-landis commented 10 years ago

@the-kenny Not that expensive actually.

@pleasetrythisathome I did not understand how ShouldUpdate could be used, thanks for the example.

@selfsame thanks for those snippets - I wasn't aware how to get a fully qualified cursor path for a given component. Very useful.

Off topic, but does anyone know how I could do the reverse - given a fully qualified cursor path, get the instance of a component?

ath commented 10 years ago

In my slider widget for om ( https://bitbucket.org/athieme/om-widgets ) you call the slider with the parent-owner:

(defn foo [cursor owner]
  (reify
    IRenderState
    (render-state [this {:keys [val]}]
      (dom/p nil
        (dom/span nil val)
        (slider :val owner))
        ...

Now when there is more state in an instance returned by foo, other than val and that state changes then my slider’s will-update & Co. get called. I will see to implement should-utpdate but wanted to add this line of thought anyway. If a child (slider instance) sees/observes state of its parent (foo instance), then the child does not necessarily always want to get informed (called) for its parent’s state changes.

swannodette commented 9 years ago

experimental support for this landed in master https://github.com/swannodette/om/commit/6890d9e4150ede6885e1de10d1b22e758c603abe