Avoid unnecessary vdom creation about purescript-pux HOT 17 CLOSED

Yeah, that works for now but this is a high priority for me as well. I've got a few ideas and will work on it soon 😄

from purescript-pux.

Hey people. I've been following this issue and like others felt like it should be possible to do something to improve performance. Initially I also thought shouldComponentUpdate might be the key, but it's too tied in to the react state shenanigans.

Just when I was about to give up an idea hit me. I started to effectively re-implement the following Signal function(s) in Pux, and then realising it's exactly what we want, only we don't currently expose the Signal state to view.

• dropRepeats :: forall a. (Eq a) => Signal a -> Signal a
• dropRepeats' :: forall a. (Signal a) -> (Signal a)

#54

If you run the hacked pair-of-examples you'll see that the top counter row is cached and only traces it's html updates if the counter changes. I'd love to see an example of someone using this branch in anger and whether it makes a difference for a Real World example. I must confess we've only just started using Pux but performance hasn't been a concern, yet, so to be clear I'm doing this out of interest not real need.

Other than any actually implementation problems, which may well exist, my biggest concern is that the elegant view :: state -> Html action becomes an uglier and less user-friendly view :: Signal state -> Signal (Html action). That said all that is required is map to lift the old form when creating Config, so perhaps that's acceptable?

It may well be a dumb idea, but figured it couldn't hurt to share. Thoughts?

from purescript-pux.

I'm pretty sure returning false from shouldComponentUpdate just keeps the existing vdom, which is quickly removed when the diff is created from the new and current vdom.

from purescript-pux.

Also I don't really know what Pux is doing but the existing type that comes to mind is Lazy. Could Html be renamed to ReactHtml (or something) and Html be an alias for Lazy ReactHtml?

If Pux does only creates one React component it can't use the existing React functionality for doing partial tree updates.. maybe instead of re-implementing that stuff in Pux we should work out how make each view a React component that uses the pure-render plugin.

from purescript-pux.

If Html was changed to Lazy Html, how would you determine if you should run it? Pux would have to keep track of a State -> Html map and if the state is different, then evaluate the Lazy Html and update the map. But Pux doesn't control what state is passed in to each view (and that's a good thing since I find myself passing in other things than just state to my update/view).

Making everything a React component and using pure-render does solve the problem if props can be memoized in the same way as above since they would have to be exactly the same object to work with pure-render.

from purescript-pux.

What about a magic component that's basically a div? I realized once I finished that it doesn't need to be a div as long as it requires the callback to return a single component (which Html a would do), so ignore this.

You could have the choice to replace this:

view state =
  div [] [ text $ "hello " ++ state.name ]

with this:

view = component \state ->
  div [] [ text $ "hello " ++ state.name ]

component would have the type Eq state => (state -> Html a) -> state -> Html a, and would look something like this (JavaScript): (renderFn) => React.createElement(factory({shouldComponentUpdate: eqInstanceImpl, render: renderFn}))

The exact Pux render implementation is a little fuzzy to me still so I'm not quite sure if that works.. you might need some puxParentAction prop copying trickery. I haven't used cloneElement before.

from purescript-pux.

Hi nice people,

I've also run into this issue of non-ideal vdom behavior; I have one subview who's rendering takes a while (2 seconds) and blocks the browser thread. I'm fine with this behavior when a submit button is clicked once in a while for example, but currently any changes to the global state ends up re-rendering that expensive component, which makes interactions like typing into a text box unusable (2 second wait times between character entries).

The way I've been able to mitigate that is to put the expensive subcomponent into a native React component with a shouldComponentUpdate and then call via FFI, but this seems non-ideal...

from purescript-pux.

@spicydonuts That looks nice, basically a memoizing view function? The problem is for records which are used a lot for state in pux, PS does not allow definition of type class instances. Fortunately it is also not needed!

Actually we could let PureScripts purity really shine here: If we changed the EffModel to not contain State, but a Maybe State. Then we could model non state changing actions with passing Nothing.

• Parent components would then simply also pass Nothing if their child returns Nothing.
• Pux would simply use the old state when it encounters Nothing

This way Parent components would then not needlessly update their State on every child action - which is good in itself, as we can avoid needless records updates. But it does not stop there! By using a Maybe State, only updating containing states on actual changes we would have the very nice property that everytime the State is replaced - the replacement is actually different! That further means we could use JavaScripts === operator for the state objects in your component function, which checks reference equality, which would be perfectly sufficient in this model and blazingly fast.

So we could improve performance a lot:

• No needless state record updates!
• Very fast checking of state differences
• No needless vdom recreation
• Very fast vdom diffs for (unchanged) trees. (I assume react also compares addresses here.)
• Even if a parent component changes, it's children's vdoms would not need to be recreated.
• if pux receives Nothing for state, it can skip the call the react entirely

In summary: I like :-)

from purescript-pux.

@dkoontz I pass in other things then state too. But this would not need to be a problem as we could easily provide a component2 method (name debateable) which takes two arbitrary parameters. (One being state, the other what ever you like.) Also component would be opt in - you don't have to use it in every component.

from purescript-pux.

@eskimor That's an interesting idea. I don't really like re-implementing what React already does though..

I've also been spending more time in Redux lately and it's kind of changing the way I think about all this. I'm used to having one immutable state that gets passed down through the whole tree, but that isn't how it works in Redux. Instead, each section of the app that needs some part of the global app state opts into it and provides a mapping function from global to local state (the props of that component). This way that component remains simple and prop-based, and its parent doesn't do anything special. This is also a great place to implement pure-render by default, since it's where most change happens. It doesn't matter much for most apps whether the tree below that point is using pure-render and full components or if it's just simple functions (like pux is now, or like <div>{someComponent({props})}</div> in JS instead of <div><SomeComponent ...props /></div>). There are exceptions to this of course, particularly with large lists or components you know will never change, but I think it's reasonable to have helper functions for those that basically act as decorators over that section of the tree. Does that make sense? I'm also not sure if the way I'm wanting to do it now fits into Pux, which is why I haven't done much on this.

from purescript-pux.

@spicydonuts Thanks for the pointer to redux, that is indeed an interesting approach.

from purescript-pux.

Redux uses one immutable state passed down through the tree, but uses "reducer" functions to expose a subset of that state to each view. ReactRedux.connect() takes these reducers and wraps the component.

You could do the same thing with Pux:

module App.State where

type AppState = { foo :: String, bar :: String }

module App.Component where

import App.State (AppState)

type State = { bar :: String }

reduce :: AppState -> State
reduce appState = { bar: appState.bar }

-- could be abstracted into wrapper function analogous to ReactRedux.connect()
view :: AppState -> Html Action
view appState =
  let
    state = reduce appState
  in
    div [] [ text state.bar ]

from purescript-pux.

It's close, but you still have to pass the state down from parents and wouldn't benefit the way react-redux does from using context to transfer state down the tree (to bypass re-rendering of components which don't care about the changed portion of the state.

from purescript-pux.

Thumbs up for an actual implementation of your approach, I wished I had time for this right now.
I have to think about this a little more, but what's not immediately obvious to me is how nested components are supposed to work with your approach.

On first glance I find my approach a bit more elegant - but I am obviously biased ;-)

from purescript-pux.

@alexmingoia you said on the gitter on Sept 19 that the 6.0.0 release fixed the pure rendering problem? I didn't seem to find it in the changelog. Or was it delayed for a later release?

from purescript-pux.

It was delayed, as I haven't had the time to do testing and benchmarking - there's a lot of refactoring.

If redundant view rendering is really a performance issue, you can wrap views with purescript-lazy.

from purescript-pux.

Pux 8 has a memoize function that caches views. Additionally, Pux 8 supports custom renderers so any kind of optimization strategy can be implemented if desired.

from purescript-pux.