render: wrap stateResult and attrsResult in Promise.resolve(), fix #2592

[kfule]

Description

This PR wraps the return value of onbeforeremove in Promise.resolve(). This ensures that thenable objects are also always processed asynchronously.

Motivation and Context

fix #2592.
The current implementation of mithril calls finally() instead of then() in onbeforeremove process, so that returning a simple thenable object in onbeforeremove will throw an error.

How Has This Been Tested?

• npm run test including additional and modified tests.
  • In this PR, I modified the onremove complex test to pass. The test as a whole seemed to assume a synchronous process, including onbeforeremove.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My code follows the code style of this project.
• I have added tests to cover my changes.
• All new and existing tests passed.
• My change requires a documentation update, and I've opened a pull request to update it already:
• I have read https://mithril.js.org/contributing.html.

[dead-claudia]

Edit: misread the original issue.

I'd like to see the section of code cleaned up a bit, especially since it can now exploit the fact the callback's truly only ever called once. This would be better IMO for that section.

// This separation naturally ensures that the remove finalizer callback is only
// created if removal is actually blocked.

function tryBlockRemove(parent, vnode, source, counter) {
    if (typeof source.onbeforeremove !== "function") return

    var original = vnode.state
    var result = callHook.call(vnode.state.onbeforeremove, vnode)

    if (result == null) return

    Promise.resolve(result).finally(() => {
        checkState(vnode, original)
        tryResumeRemove(parent, vnode, generation, counter)
    })

    counter.v++

    generation = currentRender
    for (var dom of domFor(vnode)) delayedRemoval.set(dom, generation)
}

function tryResumeRemove(parent, vnode, generation, counter) {
    if (!--counter.v) {
        onremove(vnode)
        removeDOM(parent, vnode, generation)
    }
}

function removeNode(parent, vnode) {
    var counter = {v: 1}
    if (typeof vnode.tag !== "string") tryBlockRemove(parent, vnode, vnode.state, counter)
    if (vnode.attrs) tryBlockRemove(parent, vnode, vnode.attrs, counter)
    tryResumeRemove(parent, vnode, generation, counter)
}

[dead-claudia]

Separately, if you're feeling up for it (you very much don't have to), I'd be happy to see that domFor helper and associated delayedRemoval map moved into render and optimized to not allocate or do quite as much.

var delayedRemoval = new WeakMap()

function eachDOM(vnode, fn, arg, generation) {
    var dom = vnode.dom
    var domSize = vnode.domSize
    
    while (domSize) {
        var nextSibling = dom.nextSibling
    
        if (delayedRemoval.get(dom) !== generation) {
            fn(dom, arg)
            domSize--
        }
    
        dom = nextSibling
    }
}

// ...

function appendChild(dom, target) {
    target.appendChild(dom)
}

function moveDOM(parent, vnode, nextSibling) {
    var target = vnode.dom
    if (target != null) {
        // don't allocate for the common case
        if (vnode.domSize > 1) {
            target = getDocument(parent).createDocumentFragment()
            eachDOM(vnode, appendChild, target)
        }
        insertDOM(parent, target, nextSibling)
    }
}

// ...

function markGeneration(dom) {
    delayedRemoval.set(dom, generation)
}

// From my earlier change request
function tryBlockRemove(parent, vnode, source, counter) {
    // ...

    generation = currentRender
    eachDOM(vnode, markGeneration)
}

// ...

function removeChild(dom, parent) {
    parent.removeChild(dom)
}

function removeDOM(parent, vnode, generation) {
    eachDOM(vnode, removeChild, parent, generation)
}

// ...

This would also imply setting vnode.domSize = 1 where it's not already set to something else (read: in the text node and element factories and updaters).

[kfule]

@dead-claudia Thanks for your review comments.
I cleaned up removeNode() based on your code snippet. The current behavior of domFor seems to require that the following process be limited to when counter.v === 1.

for (var dom of domFor(vnode)) delayedRemoval.set(dom, generation)

Also, I personally agree with moving the domFor helper to render, but do you mean to delete the public API m.domFor()?
If you don't mind, I would like to be considered for another PR.

[dead-claudia]

Also, I personally agree with moving the domFor helper to render, but do you mean to delete the public API m.domFor()?
If you don't mind, I would like to be considered for another PR.

@kfule That function haa never been exported. It's an implementation detail.

[kfule]

@dead-claudia It appears that the user can access the domFor helper as m.domFor(). Although I think the useful cases are very limited.

mithril.js/mithril.js

Line 1748 in e57011c

m.domFor = domFor.domFor

[dead-claudia]

@kfule I had no idea that was being exported. Worth noting this function has never been documented.

There is a use case, though, and it normally looks like this:

return m.fragment({
    oncreate(vnode) {
        const inputElems = m.domFor(vnode)
    },
}, things.map(thing => m("input", {
    // ...
}))

So we could keep it around and just modify it so it can work correctly in onbeforeremove callbacks (read: if (vnode.domSize) generation = delayedRemoval.get(vnode.dom) instead of accepting it by option).

In any case, keeping it as-is isn't a good idea. Our needs differ from what would be useful in the public API.

[kfule]

@dead-claudia
As you mentioned, there also seems to be a problem with leaving it public.

• The domFor in onremove() does not work well during delayed removal. (flems)
• When there is onbeforeremove() in both vnode.state and vnode.attrs, domFor in onbeforeremove() in the latter vnode.attrs does not work well (since this PR.) (flems)

What you wrote "generation = delayedRemoval.get(vnode.dom) instead of accepting it by option" seems like a good solution. It also seems to reduce the amount of code a bit.
I will try to submit another PR to fix them.