Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Subtree rendering proposal #1907

Closed
dead-claudia opened this issue Jul 19, 2017 · 22 comments
Closed

Subtree rendering proposal #1907

dead-claudia opened this issue Jul 19, 2017 · 22 comments
Labels
Area: Core For anything dealing with Mithril core itself Type: Breaking Change For any feature request or suggestion that could reasonably break existing code Type: Enhancement For any feature request or suggestion that isn't a bug fix
Milestone

Comments

@dead-claudia
Copy link
Member

dead-claudia commented Jul 19, 2017

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:

  • Use m.mount(vnode.dom, ...) to set up a subtree. (Possible today)
  • m.redraw(root), m.redraw.sync(root) - Redraw just this root, rather than all roots.
  • Constraint: If the root to render equals or contains the one currently being rendered, it should throw.
    • This check can be trivially added in render/render.js using root.contains(current).

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

@dead-claudia dead-claudia added Type: Breaking Change For any feature request or suggestion that could reasonably break existing code Type: Enhancement For any feature request or suggestion that isn't a bug fix labels Jul 19, 2017
@dead-claudia dead-claudia added this to the 2.0.0 milestone Jul 19, 2017
@dead-claudia
Copy link
Member Author

dead-claudia commented Jul 19, 2017

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
Copy link
Contributor

spacejack commented Jul 20, 2017

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
Copy link
Member Author

@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
Copy link
Contributor

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
Copy link
Member Author

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

@kvanbere
Copy link

kvanbere commented Jul 24, 2017

How about

  • m.redraw('path.to.key')
  • m.redraw('rootkey.*.key1.*.key2') to deepdive down subnodes
  • m.redraw('..') redraw parent ?

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
Copy link
Member Author

@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:

  • Streams don't implicitly unwrap promises, nor does it have a then method at all.
  • You can observe changes to streams, unlike with props.
  • Streams otherwise work fairly similarly to props.

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
Copy link
Member Author

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
Copy link

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
Copy link

@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
Copy link
Member Author

dead-claudia commented Sep 18, 2017 via email

@mnichols
Copy link

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
Copy link
Member Author

dead-claudia commented Sep 18, 2017 via email

@mnichols
Copy link

@isiahmeadows Done! dead-claudia/mithril-helpers#1

@valtron
Copy link
Contributor

valtron commented Oct 16, 2017

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
Copy link
Member Author

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

  • Subtree components are components whose view returns an m.subtree vnode (internally, vnode.tag === "!")
  • m.subtree(children) - Create a subtree vnode from a vnode tree (basically, whatever the view returns). It is only valid as a component's top-level vnode.
  • vnode.root - This is a reference to the next-highest vnode subtree root, or cyclicly vnode if it's a known root.
  • m.redraw(elem) - Redraw this root element. This does not require m.mount to work.
  • m.redraw(vnode) - Redraw the root vnode of a component or DOM vnode. This does not directly redraw the DOM, instead delegating to vnode.root.state.onredraw(vnode.root) (with obvious batching).
  • component.onredraw(vnode) - This is called on each child redraw, with the root's vnode. The default is to call m.redraw(vnode.dom).
  • m.redraw would now integrate with m.render, without requiring m.mount to work. This is pretty similar to how React works, although it's simpler and lower-level.
  • Event handler auto-redrawing invokes m.redraw(vnode) rather than performing a global redraw.

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

  • m.mount would be changed to work like this (where renderService.subscribe and renderService.unsubscribe are here, and renderService.render is basically m.render):

    m.mount = function (redrawService) {
        return function (elem, Comp) {
            if (Comp != null) {
                redrawService.subscribe(elem)
                redrawService.render(elem, [m({
                    view: function () { return m.subtree(m(Comp)) },
                })])
            } else {
                redrawService.unsubscribe(elem)
                redrawService.render(elem, [])
            }
        }
    }

    (Another way of seeing it is that it'd effectively replace m.mount in a more scalable, flexible way.)

  • m.render's internal setEventCallback would receive the root vnode as well as the element and event.

  • m.render would allow nested renders if the currently rendered element is a direct parent of the passed element. It would be checked here instead of m.redraw.

  • m.render's internal service would expose the ability to get the currently rendered vnode.

  • m.request would be modified to capture the currently rendered vnode and redraw it, or optionally accept a background: vnode to redraw a vnode upon completion (instead of m.request(...).then(function () { m.redraw(vnode) }).

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

  • It'd immediately obsolete my SelfSufficient utility with a much easier, more intuitive solution.
  • It does open the door to eventually deprecating/removing m.mount and global redraws, which should simplify our API some and avoid some edge cases.
  • It'll simplify integrating our routing system to redraws.

@dead-claudia
Copy link
Member Author

Couple other clarifications:

  • m.redraw(vnode) would be a no-op if vnode.dom is undefined, in case someone uses stream.map(() => m.redraw(vnode)), where stream is a persistent reference.
  • It would also have the side effect of making performance more predictable for subtree redraws, since it can hook directly into the renderer.
  • It may require migrating the redraw system into m.render's internal implementation for performance reasons.

@valtron
Copy link
Contributor

valtron commented Oct 17, 2017

(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
Copy link
Contributor

tivac commented Oct 17, 2017

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
Copy link
Member Author

@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:

  • Could there be a way to control which event callback is used for each render, at a fine-grained level (as opposed to the global-only renderService.setEventCallback)? This could be as simple as an extra argument or something.
  • Could we also pass the vnode in question to that callback?

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
Copy link
Member Author

dead-claudia commented Oct 17, 2017

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
Copy link
Member Author

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

  • Access to redrawService to subscribe/unsubscribe temporary roots
  • Ability to render with a custom onevent callback
  • Ability to cancel requested redraws (preferably in a ref-counted sort of way)

@tivac tivac modified the milestones: 2.0.0, post-v2 Apr 24, 2018
@dead-claudia dead-claudia mentioned this issue Jul 7, 2019
11 tasks
@dead-claudia dead-claudia added the Area: Core For anything dealing with Mithril core itself label Sep 2, 2024
@dead-claudia dead-claudia moved this to In discussion in Feature requests/Suggestions Sep 2, 2024
@MithrilJS MithrilJS locked and limited conversation to collaborators Sep 2, 2024
@dead-claudia dead-claudia converted this issue into discussion #2944 Sep 2, 2024
@github-project-automation github-project-automation bot moved this from In discussion to Completed/Declined in Feature requests/Suggestions Sep 2, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
Area: Core For anything dealing with Mithril core itself Type: Breaking Change For any feature request or suggestion that could reasonably break existing code Type: Enhancement For any feature request or suggestion that isn't a bug fix
Projects
Status: Completed/Declined
Development

No branches or pull requests

7 participants