Separate lifecycle methods from attributes and components

[dead-claudia]

(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 😀