MithrilJS / mithril.js

A JavaScript Framework for Building Brilliant Applications
https://mithril.js.org
MIT License
13.99k stars 926 forks source link

[rewrite] Granular redraw API proposal #1165

Closed barneycarroll closed 8 years ago

barneycarroll commented 8 years ago

m.redraw( vnode )

Easy invocation, covers all use cases, minimal extra API work. What do we think?

lhorie commented 8 years ago

Some thoughts off the top of my head:

darsain commented 8 years ago

What if components would just receive redraw function bound to their scope, ready to be used when they need? Example:

var app = {
  controller(redraw) {
    redraw(); // redraws only the app on #example
  }
};
m.mount(document.getElementById("example"), app);
barneycarroll commented 8 years ago

vnode is not accessible from its own event handlers (unless you do silly closure contortions)

Not sure I understand. I'm aware adopters of the 'vm' tradition enjoy defining methods outside of scope and passing arguments back and forth, but then it's their choice and presumably they're used to the cumbersome nature of indirection. If you define logic within its relevant scope, you don't have to do that. This is proposed usage:

const Inspector = {
  oninit : vnode => {
    // Define redraw conditions internally
    vnode.attrs.focus.run( () => 
      m.redraw( vnode ) 
    )
  }
}

// Invocation
m.mount( document.body, {
  oninit : ( { state } )=> {
    // No presumption of lower order redraw concerns
    state.focus = m.prop()
  },

  view : ( { state : { focus } } ) => [
    m( Selector, { focus } ), // Writes to stream, doesn't invoke redraws
    m( Inspector, { focus } )
  ]
} )

how would an actual update work?

Pseudo-code v0/v1 mish-mash for how this would work without extending Mithril:

m.render(
  vnode.dom.parentNode, 
  vnode.dom.parentNode.children.map( child =>
    child != vnode.dom ? { subtree : retain } : this.view( vnode )
  )
)

The proposal is to extend Mithril to keep internal references so as to avoid author legwork.

@darsain this proposal does it with less API surface, which I think is a good thing. The new vnode structures are big enough as they are without stateful methods IMO!

leeoniya commented 8 years ago

I'm aware adopters of the 'vm' tradition enjoy defining methods outside of scope and passing arguments back and forth,

@barneycarroll can you clarify?

lhorie commented 8 years ago

vnode is not accessible from its own event handlers

@barneycarroll I mean this

var count = 0

m.mount(root, {
  view: function() {
    return [
      m("div", "stuff"),
      m("div", {onclick: function() { count++; /*how to redraw only this div?*/ }}, [
        "redraw me " + count + "times"
      ]),
      m("div", "more stuff"),
    ]
  }
})
gilbert commented 8 years ago

Requiring oninit to use granular redraws is a fine tradeoff IMO, considering it's not a common case.

With that said: for the sake of argument, would it be possible to expose the vnode via the event object? i.e. onclick: function(e){ m.redraw(e.vnode) }

lhorie commented 8 years ago

yes, I could put vnode in e, but that doesn't address the issue of recomputing the new vnode, i.e. the "redraw me" + count + "times" stuff in my example above

barneycarroll commented 8 years ago

@leeoniya sorry, I'm being glib. tl;dr: OOP can be very confusing :) There's no formal definition of a 'view model' in Mithril - it's a loose and ambiguous convention for structure and separation of concerns.

Mithril docs tout the virtues of view models in several places, but state management gets tough when you mix informal contracts with library-managed contracts (ie controller and view execution logic). Perhaps because this article demonstrates the value of defining static methods outside of instance constructors while this guide uses similar terminology to describe keeping stateful properties and methods elsewhere on the component object, people often conflate the 2 ideas into a 'best practice' imperative to keep logic outside of API methods (view, controller), and avoid closures in general, and stick things in objects. This then becomes a pain when you find that all the pertinent input for your components comes from within those scopes, and yet you must define the methods that make use of that input outside of scope. Most commonly, people tie themselves in knots because event handlers need references to the arguments passed into the view, but feel it's bad practice to define the handlers in the views.

@lhorie right, that's indeed impossible. And I get what you're saying now about closures now: I'm definitely not proposing this:

m("div", ( me => ( {
  oninit : vnode => me = vnode
  onclick: () => m.redraw( me )
} )(), "redraw me")

I was thinking of component vnodes specifically — perhaps I should make that clearer in the OP. I think it's reasonable to say that any complex stateful requirements need a component to be expressed clearly, and therefore non-component nodes should defer to their containing component for that kind of logic. In that scenario, we're essentially saying divs 1 & 3 are static structures, which we can cater for using onbeforeupdate. I mean, we could technically do this with closures (aren't closures amazing?), but I don't really see the use case TBH.

gilbert commented 8 years ago

yes, I could put vnode in e, but that doesn't address the issue of recomputing the new vnode

Ah, I understand now. Seems like the function sig would have to look like m.redraw( vnode, viewFn ) to handle all kinds of granular redraws. But don't know how viable that is...

lhorie commented 8 years ago

@barneycarroll yeah, realistically, I think we can only support component vnodes. So, as I was saying, the challenge with components is that they can take parameters, so expanding my example, we get this:

var count = 0

var MyComponent = {
  view: function(vnode) {
    return m("div", {onclick: function() { count++; /*how do we redraw only MyComponent?*/ }}, [
      "redraw me " + vnode.attrs.count + "times"
    ])
  }
}

m.mount(root, {
  view: function() {
    return [
      m("div", "stuff"),
      m(MyComponent, {count: count}),
      m("div", "more stuff"),
    ]
  }
})

So basically the new vnode.attrs.count has to come from somewhere, probably as arguments to m.redraw, e.g. m.redraw(vnode, {count: count}).

barneycarroll commented 8 years ago

Here's my toy implementation: broken (largely because we have no redraw strategy) but I think it proves that all the crucial references are already available on the vnode itself.

@lhorie WRT the count example, this is a pass-by-value problem. I would describe the situation above as code smelly. It's a given that a local redraw cannot re-execute the call graph that resulted in its invocation. In fact, that's the whole point. What's desirable above is a global redraw. Local redraws are only useful when the component in question deems itself capable of producing a new state without any new information. On a dirt simple level, you can have locally stored state changes. My dropdown has toggled. The toggling of the dropdown is a purely private concern, and bears no relevance to my header, menu, data visualisations, etc. On a more involved level, you have the props based thing, whereby you no longer need a view graph execution to persist live data piped through streams. This last bit is essential to what you're driving at in #1166 WRT to a holistic strategy for data flow.

To elaborate on the initial example I gave above: because components can react to stream updates, I can create a stream in my top level component, pass it down to component A, which at a later point updates the stream, and have component B react to that change without any 'draw' action.

gilbert commented 8 years ago

So I think I've found a way to do granular redrawing without modifying mithril, using streams.

Here's the definition:

var GranularComponent = {

  oncreate: function(vnode) {
    GranularComponent.redraw(vnode)

    // Define redraw conditions internally
    vnode.attrs.dependency.run(function(items) {
      vnode.state.items = items
      console.log("Dependencies updated!", items)
      GranularComponent.redraw(vnode)
    })
  },

  onbeforeupdate: function () { return false },

  view: function(vnode) {
    console.log("Mounting component")
    return m('.component')
  },

  redraw: function (vnode) {
    console.log("[GranularComponent] redraw component")
    m.render(vnode.dom, [
      vnode.state.items
        ? m('ul', vnode.state.items.map( item => m('li', item) ))
        : m('p', "Loading (wait 2 seconds)...")
    ])
  }
}

To use it, all you need to do is pass in a stream...

var stream = m.prop()
// ...
m( GranularComponent, { dependency: stream } )

...and updating that stream (e.g. stream([10,20,30])) will update that one component, without triggering a global redraw :D

Here's a JS Bin of it in action.

ludbek commented 8 years ago

Granular redraw is a most have feature for better performance. It would allow one to avoid unnecessary redraws and unnecessary computations.

dead-claudia commented 8 years ago

@lhorie @barneycarroll @mindeavor I posted a possible, albeit untested, way of doing this in userland in both 0.2.x and the rewrite. I've also copied and pasted it here below.

// Common helper
function wrap(object, method, callback) {
  if (obj[method] != null) {
    const old = object[method]
    object[method] = function () {
      var ret = old.apply(this, arguments)
      callback.apply(null, arguments)
      return ret
    }
  } else {
    object[method] = callback
  }
}

// 0.2.x
// Note that this requires the view to return a literal DOM node, not a
// component node. Use like this:
//
// var component = {
//   controller: function () {
//     this.render = subtree(component)
//   },
//   // use ctrl.render()...
// }
function subtree(component) {
  const {view} = component
  let root

  component.view = function () {
    var node = view.apply(this, arguments)
    wrap(node.attrs = node.attrs || {}, "config", (elem, isInit, context) => {
      if (!isInit) root = elem
      wrap(context, "onunload", () => { root = null })
    })
    return node
  }

  return () => m.redraw(root, component)
}

// In rewrite, as an actual component. Use like this:
// m(Subtree, {
//   component: m(Component, ...),
//   render: render => vnode.state.render = render,
// })
//
// vnode.state.render() // in your component somewhere
const Subtree = {
  oninit(vnode) {
    vnode.attrs.render(() => {
      if (vnode.dom != null) m.render(vnode.dom, vnode.attrs.component)
    })
  },
  view(vnode) { return vnode.attrs.component },
}
pygy commented 8 years ago

You can get throttled, granular and global redraws by using m.mount instead of m.render.

The nearest redrawer can be obtained from this in event handlers and from vnode.dom in lifecycle hooks.

Since throttling occurs on a per mount point basis, there won't be spurious redraws.

It builds on #1172 and optionally on #1157

https://jsfiddle.net/b4b5oa97/1

The main drawback at this point is that the attributes are only passed at creation time. It may be possible to create a state shuttle between the two islands using an additional component, but it will probably require swapping two lines in render/render.js so that vnode.dom.parentNode.vnodes[0].state is accessible from oncreate. Right now, the "nested normal" component is never updated. Even if normal children were passed down on redraw, the node equality tests mean that the children would only be updated on global redraws. So, a priori, no higher level components using that technique...

Aside: The docs mention that getting the parentNode of a DOM object can cause a repaint in some browsers, which ones are concerned? If it is still relevant in modern browsers, my nearestRedrawer() could otherwise kill performance.

dead-claudia commented 8 years ago

@pygy Are you referring to my component-based (function-based in 0.2) workaround?

dead-claudia commented 8 years ago

And BTW, is that fix a PR anywhere? I'd merge that pretty quickly.

lhorie commented 8 years ago

I'm closing this in favor of @mindeavor's idea

dead-claudia commented 8 years ago

@lhorie Should we get a rewrite recipes/snippets page going?

ludbek commented 7 years ago

@lhorie I really feel we need per component redraw. Consider a page with 1k+ components. Deleting and updating components from the page is really slow with current architecture. Consider a situation where a store knows exactly which components are dependent to which data of it (store). In that situation the store could individually redraw/remove those components without diffing entire Mithril app.

Reading your comments above I found that you seem biased on way to synchronize component's attrs and state. Using the concept from @mindeavor I implemented isolated component in mithril-componentx. Please check this example. To make it work I cache the vdom from last global redraw and replace its state with latest state of the component on every granular redraw. May be we could do similar in Mithril's core?