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

Replace onbeforeupdate with m.retain() #2688

Closed
dead-claudia opened this issue May 28, 2021 · 9 comments
Closed

Replace onbeforeupdate with m.retain() #2688

dead-claudia opened this issue May 28, 2021 · 9 comments
Assignees
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

Comments

@dead-claudia
Copy link
Member

dead-claudia commented May 28, 2021

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 dead-claudia added the Type: Enhancement For any feature request or suggestion that isn't a bug fix label May 28, 2021
@dead-claudia dead-claudia self-assigned this May 28, 2021
@dead-claudia dead-claudia added the Type: Breaking Change For any feature request or suggestion that could reasonably break existing code label May 28, 2021
@dead-claudia dead-claudia added the Area: Core For anything dealing with Mithril core itself label May 28, 2021
@gilbert
Copy link
Contributor

gilbert commented May 28, 2021

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

@dead-claudia
Copy link
Member Author

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

@StephanHoyer
Copy link
Member

good old {subtree: "retain"} ❤️

@dead-claudia
Copy link
Member Author

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

@dead-claudia
Copy link
Member Author

dead-claudia commented Jun 3, 2021

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

dead-claudia commented Jun 9, 2021

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.

@gilbert
Copy link
Contributor

gilbert commented Jun 9, 2021

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

@dead-claudia
Copy link
Member Author

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

dead-claudia commented May 31, 2022

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 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 #2919 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

3 participants