MithrilJS / mithril.js

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

Subtree rendering proposal #1907

Open dead-claudia opened 7 years ago

dead-claudia commented 7 years ago

Edit: clarification Edit 2: See here for an update. Edit 3: Bad idea. Back to square one...

(follow-up for #1847)

/cc @pygy @tivac (and others interested)

Here's my current idea for subtree rendering:

This should be pretty easy to implement and should cover the general use case.

dead-claudia commented 7 years ago

We also could (and should) make m.mount synchronous to alleviate an odd performance glitch with this (and page load in general). That and the new m.redraw.sync would also fix most of the need for #1868 indirectly.

spacejack commented 7 years ago

Just wondering out loud, and probably a separate issue, but what to do with event listeners within these subtrees (which would trigger global redraws.)

EDIT: I guess my question is, is there an advantage to this over using m.render (within which event handlers do not trigger global redraws - presumably you would set up your own subtree re-render.)

dead-claudia commented 7 years ago

@spacejack It may be viable to make them only redraw the subtree they were defined in. It's pretty rare not to, and IMHO it's pretty odd that we currently do auto-redraw roots whose event handlers never fired.

EDIT: I guess my question is, is there an advantage to this over using m.render (within which event handlers do not trigger global redraws - presumably you would set up your own subtree re-render.)

Yes: you can define subtrees without having to manually redraw everything yourself. In addition, see the first part of this response for a suggested perf fix (which should affect relatively few cases).

spacejack commented 7 years ago

There may also be cases where you do want to trigger a global redraw. Is this how it would work?

onclick() {} // Triggers subtree redraw

onclick() {m.redraw()}  // Force global redraw - presumably avoids redundant subtree redraw?
dead-claudia commented 7 years ago

Yes, but it wouldn't avoid a redundant subtree redraw. (It'll be the existing dumb m.redraw() that exists today.)

kvanbere commented 7 years ago

How about

and m.redraw is somehow always relative to where it's called, i.e. if it's called from a component it only draws from that component downwards or if not called from within a component (i.e. a service) it redraws everything unless a key path is given

In our architecture we have (unavoidable, I think?) fat controllers that pipe mbs of data over websockets and then call redraws from the top-level controllers (via callback from service) when data arrives. We already debounce the mithril internal renders by about 300ms otherwise it halts up the app quite badly with the amount of data coming in, so subtree renders would be a nice to have

Our app could very well be the largest and most un-web thing built in mithril ever so I could be biased to a use-case nobody has (?). We are still running the 0. version stream FYI because the 1. refactor was too costly - for example we used m.prop religiously

dead-claudia commented 7 years ago

@kvanberendonck

There's a few problems you might not notice about that at first glance:

  1. m.redraw only works when not rendering.
  2. We don't know where m.redraw might be called, so we can't resolve the parent to resolve from.
  3. Mithril tracks identity by reference, not name, so we don't have string keys to resolve with.

Our app could very well be the largest and most un-web thing built in mithril ever so I could be biased to a use-case nobody has (?).

Mithril v1 is less web-dependent, actually. It was designed from the start to be less web-dependent for a better testing experience.

We are still running the 0. version stream FYI because the 1. refactor was too costly - for example we used m.prop religiously

You may appreciate mithril/stream, then, and you may also appreciate looking at the relevant changelog section for other things, if you haven't already. There are a few things to note with v0.2.x m.prop vs v1.x streams:

Oh, and trust me: many of us overused props in 0.2.x.


As for other things, the hardest part to migrate will likely be config, where most people have had issues addressing. And a few other things to take into account:

  1. Mithril v1 actually does already support multiple roots, and that's something my subtree rendering proposal takes advantage of.
  2. When migrating, it's easier to work bottom-up if you have to do it incrementally, since you have multiple roots, and you can use 0.2.x config functions appropriately during your migration.
  3. If this proposal gets implemented, your config functions at the intersection might end up looking like this (where Mithril v1 is aliased as m_v1):

    config: function (elem, isInit, context) {
        if (!isInit) {
            m_v1.mount(elem, {
                view: function () { return m_v1(Comp_v1) }
            })
            context.onunload = function () {
                m_v1.mount(elem, null)
            }
        } else {
            m_v1.redraw(elem)
        }
    }
dead-claudia commented 6 years ago

Regarding a workaround for this...I just came up with this on Gitter (and here's a packaged gist):

// How to structure a self-sufficient component
const Comp = {
     // Real inner view
     render(vnode) { /* ... */ },
     // View replacement - note that this *must* be a single DOM node
     view(vnode) { return m("div", this.render(vnode) },
     // `m.redraw` replacement
     redraw(vnode) { m.render(vnode.dom, this.render(vnode)) },
 }

Note that it does have potential safety/batching issues, though.

AugustBrenner commented 6 years ago

The (vnode | this).redraw() Idea seems great. I don't think it would cause a breaking change to the api, and it would take care of most of the slow redraw issues in larger trees. Just diff the subtree associated with the vnode.

mnichols commented 6 years ago

@isiahmeadows I tried out your Comp structure above and found that after calling redraw(vnode) on the component subsequent calls to m.render(topLevelDom, AppComponent) would not update the DOM although the new attrs would be inside the input vnode.

Curious, I manually called this.redraw(vnode) inside the onupdate method and the DOM would update as expected. It's almost like when you call render on a DOM node that is part of a tree mithril rendered previously mithril doesnt reconcile its DOM anymore.

dead-claudia commented 6 years ago

@mnichols You might want to check this out:

https://github.com/isiahmeadows/mithril-helpers

In particular, it has a SelfSufficient class in mithril-helpers/self-sufficient. (I made this with the sole purpose of having a single accessible set of useful utilities)

Note the unusual install commands: you install it from the GitHub repo directly rather than from the registry.

On Sun, Sep 17, 2017, 12:38 Mike Nichols notifications@github.com wrote:

@isiahmeadows https://github.com/isiahmeadows I tried out your Comp structure above and found that after calling redraw(vnode) on the component subsequent calls to m.render(topLevelDom, AppComponent) would not update the DOM although the new attrs would be inside the input vnode.

Curious, I manually called this.redraw(vnode) inside the onupdate method and the DOM would update as expected. It's almost like when you call render on a DOM node that is part of a tree mithril rendered previously mithril doesnt reconcile its DOM anymore.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/MithrilJS/mithril.js/issues/1907#issuecomment-330060855, or mute the thread https://github.com/notifications/unsubscribe-auth/AERrBNHUQvPRvB1daGjslgiVhpR8OVZmks5sjUrrgaJpZM4Oc7bV .

mnichols commented 6 years ago

Thanks @isiahmeadows ! I tried using SelfSufficient and it has the same issue I experienced earlier. I spun up a codepen here to demonstrate: https://codepen.io/nicholsmike/pen/pWjGoM?editors=1000. Notice that clicking the button changes the vnode.attrs.model state but the DOM doesnt update until you move the mouse again (causing another redraw).

dead-claudia commented 6 years ago

Ok. File a bug there, and I'll take a look at it. I mostly mimicked Mithril's behavior in writing it.

On Mon, Sep 18, 2017, 02:45 Mike Nichols notifications@github.com wrote:

Thanks @isiahmeadows https://github.com/isiahmeadows ! I tried using SelfSufficient and it has the same issue I experienced earlier. I spun up a codepen here to demonstrate: https://codepen.io/nicholsmike/pen/pWjGoM?editors=1000. Notice that clicking the button changes the vnode.attrs.model state but the DOM doesnt update until you move the mouse again (causing another redraw).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/MithrilJS/mithril.js/issues/1907#issuecomment-330139581, or mute the thread https://github.com/notifications/unsubscribe-auth/AERrBCiS4vr6N3A0b2gVZc1fJx3aku1iks5sjhFvgaJpZM4Oc7bV .

mnichols commented 6 years ago

@isiahmeadows Done! https://github.com/isiahmeadows/mithril-helpers/issues/1

valtron commented 6 years ago

I made a prototype of something similar:

https://github.com/valtron/mithril.js/commit/714aa99a32a9a6aff2f491029406220924787f31#diff-d781778804292a09ee5867356a2e37f3

It's basically an option for a rendering mode similar to (but better than) React's; requires manual change tracking (à la setState), but won't rebuild the entire vdom each redraw.

dead-claudia commented 6 years ago

While out away from things, I came up with another idea:

This would entail some other internal changes with major public-facing ramifications:

Here's a few other side effects it could have in the future:

dead-claudia commented 6 years ago

Couple other clarifications:

valtron commented 6 years ago

(1) Is it possible to avoid m.subtree and vnode.root? Ideally, I want to write (toy example):

class Node {
    view(vnode) {
        return [
            m('button', { onclick: onclick }, [vnode.attrs.key, ": ", vnode.attrs.value]),
            vnode.attrs.children.map(c => m(Node, c))
        ];

        function onclick() {
            vnode.attrs.value += 1;
            m.redraw(vnode);
        }
    }
}

m.render(document.body, m(Node, { key: 1, value: 0, children: [...] }));

(2) What about caching the vdom returned by view? In the toy example, when clicking the button, a redraw for its corresponding vnode is done, but the vdom previously returned by childrens' view should be reused.

(3) What sorts of things could onredraw be used for?

(4) Would m.redraw(dom) be needed anymore? Seems like m.redraw(dom) -> m.render(dom, dom.vnode). In my toy example, I'm basically using m.redraw(vnode) as "mark dirty, to be re-rendered next frame/whatever", and the name should indicate that. (I know this line is unnecessary, since "[e]vent handler auto-redrawing invokes m.redraw(vnode)", but I'm thinking about a more general use case where the component needs to trigger a redraw on a different component; e.g., imagine each Node also displays the sum of all its childrens' values.)

tivac commented 6 years ago

I am 100% against any solution that makes global redraw any more difficult than it is right now.

IMO it's one of Mithril's most valuable features. Being able to opt into more granular drawing is fine, but I don't want to be forced to do that.

dead-claudia commented 6 years ago

@tivac Understandable, and I personally started having concerns over it getting unwieldy now that I'm re-reading it. 😄 I do have a question, though: should my utility (link) be considered the ideal workaround, much like mithril-node-render is for server-side rendering? And if so:

Basically, I just need a way to define an island. This would make my (and my library's users') life much easier, since I would no longer need state.link in that utility.

dead-claudia commented 6 years ago

Apart from that, a way to reuse the throttler to (un)schedule simple tasks with associated vnodes would also be nice. You can look at the source and see that ~40% of it is just reimplementing m.redraw's throttling and batching logic. This basically brings me back to square one with my m.redraw(vnode.dom) proposal, except I'd really need something closer to m.redraw(root, callback) instead (with sync variant for locking), but with also m.redraw.cancel(root) to cancel.

dead-claudia commented 6 years ago

So here's the status of what I need to hook into Mithril's redraw system for my mithril-helpers/self-sufficient utility: