mithril.js icon indicating copy to clipboard operation
mithril.js copied to clipboard

Replace `onbeforeupdate` with `m.retain()`

Open dead-claudia opened this issue 4 years ago • 9 comments
trafficstars

Mithril version:

Platform and OS:

Project:

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

Description

Replace this pattern:

const Comp = {
    onbeforeupdate(vnode, old) {
        return cond
    },
    view(vnode) {
        return tree
    }
}

With this:

const Comp = {
    // On first render, `old` is not passed
    view(vnode, old) {
        if (!cond) return m.retain()
        return tree
    }
}

m.retain() would have a tag of "=", and that's how we'd detect. This can be used anywhere a child is, and on first render would be equivalent to undefined (you're essentially explicitly "retaining" no tree, so it's consistent).

Why

It's simpler for us to implement and simpler for users to implement. It's also more flexible.

This is something we've wanted to do for a while.

Possible Implementation

  1. Modify createVnode to do nothing on vnode.tag === "=".
  2. Modify updateVnode to, on vnode.tag === "=", transfer state much like what shouldNotUpdate does when preventing update, but also transfer tags and attributes, in effect modifying the m.retain() vnode to be the actual desired vnode. (This avoids having to replace nodes in the tree, which makes this a lot less complicated.)
  3. Remove the shouldNotUpdate check in updateNode.
  4. Modify updateComponent to invoke vnode.state.view(vnode, old).

Open Questions

dead-claudia avatar May 28 '21 00:05 dead-claudia

Love it. Super intuitive. Since it's just a vnode, would it work in any part of a tree, not just components?

gilbert avatar May 28 '21 22:05 gilbert

@gilbert Correct. It's intended to replace both the attribute the component method.

dead-claudia avatar May 29 '21 05:05 dead-claudia

good old {subtree: "retain"} ❤️

StephanHoyer avatar May 29 '21 10:05 StephanHoyer

Yup, and that's where the name came from. I miss it badly.

dead-claudia avatar May 29 '21 22:05 dead-claudia

So, in light of some recent discussion in #2690, I'd like to propose an alternative: m.updateIf(prevValue, compare, view)

const Comp = {
    view(vnode) {
        return m.updateIf(vnode.attrs, (prev, next) => cond, () => tree)
    }
}

This would operate similar to onbeforeupdate, but as a special vnode. I'd keep the same tag, though.

Conveniently, this would allow easy diffing of not only attributes, but other state, too, and unlike React, it's very flexible. It's a little more magical than m.retain(), but it provides the functionality needed, and is still reasonably easy to understand.

If you wanted a direct equivalent of React.memo, you could do this:

const hasOwn = {}.hasOwnProperty
m.memo = C => ({
    view: ({attrs}) => m.updateIf(
        Object.entries(vnode.attrs),
        (prev, next) => (
            prev.length === next.length &&
            prev.every(([k, v]) => hasOwn.call(vnode.attrs, k) && v === vnode.attrs[k])
        ),
        () => C(m.censor(attrs))
    )
})

dead-claudia avatar Jun 03 '21 22:06 dead-claudia

Okay, let me go back a little on that: that's not flexible enough for efficient DOM patching. It only really helps with the diffing (part of the allure of m.retain() in the first place). It might just be better to provide separate "track previous value" and "retain" vnodes.

function Comp(ctx) {
    return m.compare(ctx.attrs, (prev, next) => cond ? tree : m.retain())
}

It's a few more characters, but it's not that much worse, and it also means that combined with #2689, we don't need to provide any "previous" functionality. Here's the above m.memo written with each (using #2690 so it's a little easier to follow the whole picture).

// `m.updateIf`
const hasOwn = {}.hasOwnProperty
m.memo = C => ({attrs}) => m.updateIf(
    Object.entries(attrs),
    (prev, next) => (
        prev.length === next.length &&
        prev.every(([k, v]) => hasOwn.call(attrs, k) && v === attrs[k])
    ),
    () => C(attrs)
)

// `m.compare(value, init)` + `m.retain()`
const hasOwn = {}.hasOwnProperty
m.memo = C => ({attrs}) => m.compare(Object.entries(attrs), (prev, next) => (
    prev == null ||
    prev.length === next.length && prev.every(([k, v]) => hasOwn.call(attrs, k) && v === attrs[k])
    ? C(attrs)
    : m.retain()
))

// Minified `m.updateIf` vs `m.compare`
const h={}.hasOwnProperty;m.memo=C=>({attrs:a})=>m.updateIf(Object.entries(a),(p,n)=>p.length===n.length&&p.every(([k,v])=>h.call(a,k)&&v===a[k]),()=>C(a))
const h={}.hasOwnProperty;m.memo=C=>({attrs:a})=>m.compare(Object.entries(a),(p,n)=>null==p||p.length===n.length&&p.every(([k,v])=>h.call(a,k)&&v===a[k])?C(a):m.retain())

I'm not tied to the name, but that's the idea. Conversely, we can just provide m.retain(), and let components figure it out, but we're Mithril and we should do something. And plus, this is a bit nicer than putting it on ctx as explained in #2690, and Comp(ctx, prevAttrs) is admittedly inconsistent at best and just ugly in general.

dead-claudia avatar Jun 09 '21 03:06 dead-claudia

I have an idea for an api but first I need to know: how does m.updateIf / m.compare keep track of previous values?

gilbert avatar Jun 09 '21 07:06 gilbert

@gilbert It'd require a node slot (like vnode.state). Similar to how onbeforeupdate keeps track of previous vnodes, but instead of passing the whole vnode, it's just passing a single value.

dead-claudia avatar Jun 09 '21 07:06 dead-claudia

Update: I'm reverting back to the original form as described in the issue for now. It's simpler, easier to wrap your head around, and more likely to get places.

dead-claudia avatar May 31 '22 05:05 dead-claudia