Separate lifecycle methods from attributes and components

[dead-claudia]

trafficstars

(Related: #2688)

Old proposal

### Description Replace these idioms:

const Comp = {
    oncreate(vnode) {
        $(vnode.dom).modal()
        if (vnode.attrs.show) {
            $(vnode.dom).modal("show")
        }
    },
    onupdate(vnode) {
        if (vnode.attrs.show) {
            $(vnode.dom).modal("show")
        } else {
            $(vnode.dom).modal("hide")
        }
    },
    onremove(vnode) {
        $(vnode.dom).modal("dispose")
    },
    view() {
        return m("div.bs-modal")
    }
}

return m("div.bs-modal", {
    oncreate(vnode) {
        $(vnode.dom).modal()
        if (vnode.attrs.show) {
            $(vnode.dom).modal("show")
        }
    },
    onupdate(vnode) {
        if (vnode.attrs.show) {
            $(vnode.dom).modal("show")
        } else {
            $(vnode.dom).modal("hide")
        }
    },
    onremove(vnode) {
        $(vnode.dom).modal("dispose")
    },
})

With this idiom:

return m("div.bs-modal",
    m.access((dom, isInit) => {
        if (isInit) {
            $(dom).modal()
        }
        if (vnode.attrs.show) {
            $(dom).modal("show")
        } else {
            $(dom).modal("hide")
        }
    }),
    m.release(dom => {
        $(dom).modal("destroy")
    })
)

m.access(...) would have a tag of "+", and m.release(...) would have a tag of "-".

Why

It's simpler and more flexible, and when combined with #2688, you also get the ability to diff and patch based on attributes for free, which isn't possible with the current framework.

Oh, and it will also provide a few perf benefits - this blog post can give you an idea why for part of it, but also not accessing dynamic properties that may or may not exist can also help a lot.

I suspect a 5-10% perf increase and a significant library size decrease out of this.

Possible Implementation

• In createNode and updateNode, if vnode.tag === "+", schedule the callback with parent and isInit set accordingly (true in createNode, false in updateNode) and treat it otherwise as equivalent to undefined.
• In createNode and updateNode, if vnode.tag === "-", treat it as equivalent to undefined.
• In removeNode, ignore vnode.tag === "+" and invoke the callback if vnode.tag === "-".
• Merge vnode.dom with vnode.state in the vnode object, and use it for both element references, component state, and access/remove callbacks.
• Strip out all the lifecycle hook logic and centralize it to those two vnodes.

Mithril version:

Platform and OS:

Project:

Is this something you're interested in implementing yourself? Yes

Description

Replace these idioms:

const Comp = {
    oncreate({dom, attrs}) {
        $(dom).modal()
        if (attrs.show) {
            $(dom).modal("show")
        }
    },
    onupdate({dom, attrs}) {
        if (attrs.show) {
            $(dom).modal("show")
        } else {
            $(dom).modal("hide")
        }
    },
    onbeforeremove({dom, attrs}) {
        return new Promise(resolve => {
            if (!attrs.show) return
            $(dom).modal("hide")
            $(vnode.dom).one("hidden.bs.modal", resolve)
        })
    },
    onremove({dom}) {
        $(dom).modal("dispose")
    },
    view({vnode}) {
        return m("div.bs-modal", children)
    }
}

const Comp = {
    view({attrs, children}) {
        return m("div.bs-modal", {
            oncreate({dom}) {
                $(dom).modal({show: attrs.show, keyboard: false})
            },
            onupdate({dom}) {
                if (attrs.show) {
                    $(dom).modal("show")
                } else {
                    $(dom).modal("hide")
                }
            },
            onbeforeremove({dom}) {
                if (!attrs.show) return
                $(dom).modal("hide")
                return new Promise(resolve => {
                    $(dom).one("hidden.bs.modal", resolve)
                })
            },
            onremove({dom}) {
                $(dom).modal("dispose")
            },
        }, children)
    }
}

With this idiom:

// With #2690
function Comp(ctx) {
    let isInitial = true
    return () => m.fragment({
        afterRender([dom]) {
            if (isInitial) {
                $(dom).modal({show: attrs.show, keyboard: false})
            } else if (ctx.attrs.show) {
                $(dom).modal("show")
            } else {
                $(dom).modal("hide")
            }
            isInitial = false
        },
        beforeRemove([dom]) {
            if (!attrs.show) return
            $(dom).modal("hide")
            return new Promise(resolve => {
                $(dom).one("hidden.bs.modal", resolve)
            })
        },
        afterRemove([dom]) {
            $(dom).modal("dispose")
        },
    }, m("div.bs-modal", ctx.attrs.children))
}


// With current component API
const Comp = {
    oninit() { this.isInitial = true },
    view({attrs, children}) {
        return m.fragment({
            afterRender([dom]) {
                if (this.isInitial) {
                    $(dom).modal({show: attrs.show, keyboard: false})
                } else if (attrs.show) {
                    $(dom).modal("show")
                } else {
                    $(dom).modal("hide")
                }
            },
            beforeRemove([dom]) {
                if (!attrs.show) return
                $(dom).modal("hide")
                return new Promise(resolve => {
                    $(dom).one("hidden.bs.modal", resolve)
                })
            },
            afterRemove([dom]) {
                $(dom).modal("dispose")
            },
        }, m("div.bs-modal", children))
    }
}

m.fragment(...) would have the same tag it normally does. This would also by side effect mean m.censor just becomes m.censor = ({key, ...rest}) => rest absent user-provided keys, though we could just as easily strip key internally like React does (the smart thing to do IMHO) and not need it anymore.

The parameter of each is actually an array of DOM nodes. And while it's technically less efficient, it's likely to be minor in practice as significant DOM work is rare, and we're talking small numbers compared to a significantly more involved algorithm plus the native browser's own updating mechanisms plus all the adapting logic between JS and native just to invoke the browser APIs - a single array allocation is nothing compared to that, just slightly more GC churn (as it's retained for a decent amount of time).

I'm leaving out onbeforeupdate from this proposal as that's covered by #2688 and is being discussed separately.

Why

It's simpler and more flexible, and when combined with #2688, you also get the ability to diff and patch based on attributes for free, which isn't possible with the current framework.

Oh, and it will also provide a few perf benefits:

• If you read this blog post, you'll find that polymorphic calls are slow. This doesn't strictly eliminate that, but it does make it possible to remove that for the common case of components.
• Not accessing dynamic methods that may or may not exist for every element, fragment, and component vnode can also go a long way. This in effect centralizes that to only a specific special fragment type that is only rarely used.

I suspect a 5-10% perf increase and a mild library size decrease out of this, based on my experience with Mithril and its performance profile.

Possible Implementation

1. Move this line to after this line.
2. Move these lines to after this line.
3. Delete these lines from createComponent and these lines from updateComponent.
4. Change updateLifecycle to instead schedule source.afterRender with an array of vnode.domSize nodes starting from vnode.dom and continuing through elem.nextSibling.
5. Remove this function and the two places it's called.
6. Change these two lines of onremove to instead invoke vnode.attrs.afterRemove and only if vnode.tag === "[" && vnode.attrs != null && typeof vnode.attrs.afterRemove === "function".

This would also make it such that vnode.state and vnode.dom are mutually exclusive, so we could merge those accordingly.

Open Questions

• Should we do this?
• Is there a better way to do it?

[StephanHoyer]

Yet another return of ancient APIs of pre 1.0 times, nice!

[JAForbes]

I really like the idea, I've been exploring custom vnode types in my code bases to support first class view reactivity and I think its a great direction. So I'm 👍

I also really dislike onupdate being a separate context to oncreate. Not that I was a big fan of isInit in 0.2x. Ideally I would have liked a create only hook, and a hook that runs create and update. Because they are different use cases and most of the time I only needed a create hook. But I've never needed just an onupdate hook without an oncreate. So that's another reason I'm onboard with m.access.

---

Is this the right issue to bikeshed names? I'm thinking m.dom( dom => ) or m.element( element => ) is a bit more explicit about what is being accessed.

I can imagine a newcomer internalizing that more easily. When they see m.access in some example code, they might wonder what is being accessed, especially if the dom target is destructured.

m.access( ({ value }) => .... ) has no visual hint that value is the InputElement::value property of a dom element. A newcomer could read that and think we're accessing a state tree.

But m.dom( ({ value }) => ... ) is sort of wonderfully obvious and boring in all the best ways.

I'd prefer m.dom to m.element for a few reasons, but curious what others think.

But other than that I think it's great!

[CreaturesInUnitards]

return m("div.bs-modal",
    m.access((dom, isInit) => {
        if (isInit) {
            $(dom).modal()
        }
        if (vnode.attrs.show) {
            $(dom).modal("show")
        } else {
            $(dom).modal("hide")
        }
    }),
    m.release(dom => {
        $(dom).modal("destroy")
    })
)

@isiahmeadows in this context, where is the vnode ref coming from?

[dead-claudia]

@CreaturesInUnitards It's in a hypothetical component - I just left it out for brevity (and I didn't feel like spending too much time figuring out API structure and such). If we went with #2690, it'd use ctx.attrs.show instead, but same difference.

[CreaturesInUnitards]

@isiahmeadows I get that, but you're proposing a major, breaking change — "API structure and such" are everything. The merits of this proposal are almost entirely contingent on the API, specifically what's in scope OOTB. So if, for example, the thought was #2690 and the ref is context, I'm a hard no.

[dead-claudia]

@CreaturesInUnitards I get that - note the tags. 😉

I'm using separate issues as these proposals are technically separable in a sense, even if that's not my ultimate intent. I'm also trying to keep it accessible to those not actively following the discussion.

[CreaturesInUnitards]

@isiahmeadows I didn't mean to suggest you didn't know these are breaking changes 😀

[dead-claudia]

Okay, redid the proposal to sit a little closer to a happy middle, based on discussion both in Gitter and here in various issues. You can read the updated description. TL;DR: I'm basically locking all the lifecycle magic down to m.fragment.

I also corrected a bug where I misremembered the relevant Bootstrap modal method name.

In Bootstrap v5, it'd look more like this:

// With #2690
function Comp(ctx) {
    let modal
    return () => m.fragment({
        afterRender([dom]) {
            if (modal == null) {
                modal = new bootstrap.Modal(dom, {keyboard: false})
                if (ctx.attrs.show) modal.show()
            } else {
                if (ctx.attrs.show) {
                    modal.show()
                } else {
                    modal.hide()
                }
                modal.handleUpdate()
            }
        },
        beforeRemove([dom]) {
            if (!attrs.show) return
            modal.handleUpdate()
            modal.hide()
            return new Promise(resolve => {
                dom.addEventListener("hidden.bs.modal", resolve, {once: true})
            })
        },
        afterRemove([dom]) {
            modal.dispose()
        },
    }, m("div.bs-modal", ctx.attrs.children))
}


// With current component API
function Comp() {
    let modal
    return {
        view: ({attrs, children}) => m.fragment({
            afterRender([dom]) {
                if (modal == null) {
                    modal = new bootstrap.Modal(dom, {keyboard: false})
                    if (attrs.show) modal.show()
                } else {
                    if (attrs.show) {
                        modal.show()
                    } else {
                        modal.hide()
                    }
                    modal.handleUpdate()
                }
            },
            beforeRemove([dom]) {
                if (!attrs.show) return
                modal.handleUpdate()
                modal.hide()
                return new Promise(resolve => {
                    dom.addEventListener("hidden.bs.modal", resolve, {once: true})
                })
            },
            afterRemove() {
                modal.dispose()
            },
        }, m("div.bs-modal", children))
    }
}

[dead-claudia]

@barneycarroll Updated it with the old proposal's text. Took a little bit of hackery, but it got done.

[dead-claudia]

Update: this should be a new primitive vnode with its own dedicated tag and name, to keep it out of the hot path of updates, as fragments are extremely common. The shape of the proposal still remains, so I'm not going to update names of everything (yet).

[pygy]

@dead-claudia given that children are normalized to arrays, and that we want hooks to support fragments, how would the representation and handling of m.hook({}, ...children) differ from those of m.fragment({}, ...children)?

Edit: you'd use it as siblings maybe? [a, b, m.hook({})]

[dead-claudia]

@pygy I'll start off by answering this question, even though it's arguably moderately pointless with what I'm about to follow it up with.

given that children are normalized to arrays, and that we want hooks to support fragments, how would the representation and handling of m.hook({}, ...children) differ from those of m.fragment({}, ...children)?

I left that intentionally unspecified, but if they're designed to work as fragments, then yes, that's the intent.

---

The sibling idea is one I've played around with for quite a while (years, really). They'd exist as siblings, executed/scheduled whenever visited, and they would receive whatever element is their nearest parent. Of course, this is a bit radical in the world of virtual DOM, but that of course could go without stating.

I was torn on how much I liked it, because there's definitely some strong benefits, yet cons that get as large as you logically take it.

• It's a very intuitive atom to reason about, the more you think about it, and it's easy to imagine how it could be expanded. In particular, I also considered a hypothetical framework that extended this even to attributes and event listeners. This of course grants massive power - all of a sudden you could literally define animations as components and throw them inside whatever element you wanted to animate.
• However, that immense power is also an absolute nightmare for performance, because you have to virtualize not just the core DOM tree, but also attributes' underlying structure. (We currently only virtualize their values, only virtualizing the structure of the core DOM tree.) There's really only one option here: screw over the common case's performance by creating a set to track all the attributes added in a given pass. This could be recovered in large part by optimizing for the case where only one attribute set is used (in which we can just save that one attribute object and fall back to what we currently do), but that both requires significant bloat and still doesn't fully address the performance issue.