Recursive fragment w/ lost lastChild during target update

lazex commented 10 months ago

I get an error in complex cases.

<!DOCTYPE html>
<meta charset="utf-8" />

<script type="module">
    import { render, html } from "https://cdn.jsdelivr.net/npm/uhtml@4.3.4/index.js"

    const tpl = (tree, state) => {
        const item = tree.find(x => x.name === state[0])
        return html`
            <div>
                ${tree.map(item => html`<span>${item.name}</span>`)}
            </div>
            ${item?.children
                ? tpl(item.children, state.slice(1))
                : null}
        `
    }

    const tree = [
        {
            name: "A",
            children: [
                {
                    name: "A-A",
                    children: [
                        {
                            name: "A-A-A",
                            children: [
                                { name: "A-A-A-A" }
                            ],
                        },
                    ],
                },
            ],
        },
        {
            name: "B",
            children: [{ name: "B-A" }],
        },
    ]

    const case1 = () => {
        const update = (state) => render(root1, tpl(tree, state))

        update(["A"])
        update(["A", "A-A"])
        update(["A", "A-A", "A-A-A"])
        update(["B"]) // ERROR
    }
    const case2 = () => {
        const update = (state) => render(root2, tpl(tree, state))

        update(["A"])
        update(["A", "A-A"])
        // update(["A", "A-A", "A-A-A"])
        update(["B"]) // NO ERROR
    }
    const case3 = () => {
        const update = (state) => render(root3, tpl(tree, state))

        // update(["A"])
        // update(["A", "A-A"])
        update(["A", "A-A", "A-A-A"])
        update(["B"]) // NO ERROR
    }

    for (const case_ of [case1, case2, case3]) {
        try {
            case_()
        } catch (err) {
            console.error(`[${case_.name} error]`, err)
        }
    }
</script>

<div id="root1"></div>
<div id="root2"></div>
<div id="root3"></div>

case1 gives the following error.

DOMException: Failed to execute 'setEndAfter' on 'Range': the given Node has no parent.

However, it does not occur in case2 and case3.

This can be avoided by wrapping the top level of the html returned by the tpl function in a div.

const tpl = (tree, state) => {
    const item = tree.find(x => x.name === state[0])
    return html`
        <div>
            <div>
                ${tree.map(item => html`<span>${item.name}</span>`)}
            </div>
            ${item?.children
                ? tpl(item.children, state.slice(1))
                : null}
        </div>
    `
}

WebReflection commented 10 months ago

You can’t pass array or null conditionally, you can pass array or [] though. See if this helps

lazex commented 10 months ago

I tried the following code.

const tpl = (tree, state) => {
    const item = tree.find(x => x.name === state[0])
    return html`
        <div>
            ${tree.map(item => html`<span>${item.name}</span>`)}
        </div>
        ${item?.children
            ? tpl(item.children, state.slice(1))
            : [] // changed
        }
    `
}

No errors occurred, but the results were not what I expected.

Actual HTML:

<div id="root1">
    <div>
        <span>A</span><span>B</span><!--isµ0-->
    </div>
    <!--isµ1-->
</div>

Expected HTML:

<div id="root1">
    <div>
        <span>A</span><span>B</span>
    </div>
    <div>
        <span>B-A</span>
    </div>
</div>

"B-A" is not shown.

lazex commented 10 months ago

I have created a more simplified code that can be reproduced.

<!DOCTYPE html>
<meta charset="utf-8" />

<script type="module">
    import { render, html } from "https://cdn.jsdelivr.net/npm/uhtml@4.3.4/index.js"

    const tpl = (items) => {
        const [first, ...rest] = items
        return html`
            <div>${first}</div>
            ${
                // html or null
                rest.length > 0 ? tpl(rest) : null
            }
        `
    }

    const tpl2 = (items) => {
        const [first, ...rest] = items
        return html`
            <div>${first}</div>
            ${
                // html or []
                rest.length > 0 ? tpl2(rest) : []
            }
        `
    }

    const case1 = () => {
        const update = (state) => render(root1, tpl(state))

        update(["A", "B"])
        update(["A", "B", "C"])
        update(["A", "B", "C", "D"])
        update(["X", "Y"]) // ERROR
    }
    const case2 = () => {
        const update = (state) => render(root2, tpl(state))

        update(["A", "B"])
        update(["A", "B", "C"])
        // update(["A", "B", "C", "D"])
        update(["X", "Y"]) // NO ERROR
    }
    const case3 = () => {
        const update = (state) => render(root3, tpl(state))

        // update(["A", "B"])
        // update(["A", "B", "C"])
        update(["A", "B", "C", "D"])
        update(["X", "Y"]) // NO ERROR
    }
    const case4 = () => {
        // use tpl2
        const update = (state) => render(root4, tpl2(state))

        update(["A", "B"])
        update(["A", "B", "C"])
        update(["A", "B", "C", "D"])
        update(["X", "Y"]) // NO ERROR, but it renders only X
    }

    for (const case_ of [case1, case2, case3, case4]) {
        try {
            case_()
        } catch (err) {
            console.error(`[${case_.name} error]`, err)
        }
    }
</script>

<div id="root1"></div>
<hr/>
<div id="root2"></div>
<hr/>
<div id="root3"></div>
<hr/>
<div id="root4"></div>

WebReflection commented 10 months ago

Ok, in the release notes it's pretty clear you need to pass holes.

~~This is invalid:~~

rest.length > 0 ? tpl(rest) : null

~~If you have an array the fallback should be an empty array.~~

~~If you have a hole the fallback should be:~~

~~Passing null or void as content is allowed only for text nodes ... the library doesn't invent emptyness (or at least not anymore in v4).~~

Can you please try to see if never falling back to an unexpected value works?

lazex commented 10 months ago

I tried the following code.

${rest.length > 0 ? tpl(rest) : html``}

But I got the following error.

TypeError: Failed to execute 'setStartAfter' on 'Range': parameter 1 is not of type 'Node'.

WebReflection commented 10 months ago

out of curiosity ... this seems to be a fragment related issue ... what if you wrap all your fragments within a div? do you still have the issue? if not, I know what's the issue ... if yes, I need to really understand what's your intent/expectations and having a single conditional would help me smooth out the outcome. Sorry not much extra time so any extra help is welcome.

lazex commented 10 months ago

I wrapped all the fragments in divs, but an error occurs.

TypeError: Failed to execute 'setStartAfter' on 'Range': parameter 1 is not of type 'Node'.

Here is the code:

<!DOCTYPE html>
<meta charset="utf-8" />

<script type="module">
    import { render, html } from "https://cdn.jsdelivr.net/npm/uhtml@4.3.4/index.js"

    const tpl = (items) => {
        const [first, ...rest] = items
        return html`
            <div>
                <div>${first}</div>
                ${rest.length > 0 ? tpl(rest) : html``}
            </div>
        `
    }

    const update = (state) => render(root1, tpl(state))

    update(["A", "B"])
    update(["A", "B", "C"]) // ERROR
</script>

<div id="root1"></div>

WebReflection commented 10 months ago

actually the hint to use empty html was my mistake ... null would work in this latter case, or an empty string to signal an empty text content.

I am going to check other cases

WebReflection commented 10 months ago

~~OK, this is still invalid:~~

${item?.children
                ? tpl(item.children, state.slice(1))
                : null}

~~or better, it works within a node but it cannot work as child node of a fragment.~~

edit that was not the issue, which is now properly described as issue description.

WebReflection commented 10 months ago

digging more into it ... there's something too smart around the null case as that node gets lost in fragments ... I am not sure that's due too nested logic and I will find the culprit at some point but ... I think your use case is fairly edge ... nested fragment recursion doesn't look like often used in UI, it's usually always within a container but if you came here with this example I am sure you have a reason to use that pattern.

For now, I can say that not "abusing" fragments that way works and when this scenario is desired using a container is likely a better way to present, or even style, the layout.

I'll keep this open but I won't fix this too soon, thanks.

lazex commented 10 months ago

I wanted to make the HTML flat for styling purposes without nesting it, so I used fragments. I will wrap it in a div and adjust the CSS. Thanks.

WebReflection commented 10 months ago

FWIWI I've checked v3 and indeed everything works as expected in there ... this might be a regression due completely new logic implemented in v4. I want these cases to work as well as they did before but I don't have too much extra time to actually figure out why the current logic wouldn't work here ... it's a great issue report after all, and a heck of a regression from the library side but all other demos I have work great so I couldn't fully get the error first.

Useless to speculate as what's the culprit but I think the smart template "pre-parsed" upfront might be it ... I hope I'll come back with a better answer, and I am also still working on creating the minimal representation case for the issue ... as in: just one simple recursive case that fail, but the combinations in there might also be misleading so I can't narrow down the real issue.

This is just to tell you: bear with me, I've realized these shenanigans are essential to have a stable v4 for all cases, but it looks like I'm not there yet, and this is none of users fault, or expectations.

Use v3 if that worked well to date, as that will keep working well "forever" too. I will ping in here once I've got all your examples working without surprises.

WebReflection commented 9 months ago

I did investigate and I am leaning toward this conclusion ... see this comment https://github.com/WebReflection/uhtml/issues/103#issuecomment-1892690035 and the following one ... for instance, your issue with fagments in fragments can easily be solved like this:

    const tpl = (items) => {
        const [first, ...rest] = items
        return html`
            <div>${first}</div>
            ${rest.map(tpl)}
        `
    }

Should I make the code slower and the interpolation intent ambiguous when arrays (growing/shrinking list of nodes) actually solve all of this? I am thinking every second more I shouldn't and I'd rather explain what is the issue and why there is such issue.

I could fix it in code but every time I try the performance penalty is horrendous and all to avoid people shooting themselves in their foot.

WebReflection commented 9 months ago

If interested in why that would work here the answer: that code would translate into this static template:

<div><!--0--></div>
<!--1-->

That 0, being just a hole-like (string, null, undefined, or a hole) will be either the text node or the hole it represents.

The 1 though, passes through udomdiff with a pinned content and a collection of nodes to handle.

With recusrive fragment, that collection will look like this:

<div><!--0--></div>
<div><!--0--></div>
<div><!--0--></div>
<!--1--><!--1--><!--1-->

Every node in it would be pinned/related to the array comment instead of being just a persistent fragment because persistent fragments can exist but cannot have nested persistent fragments in them because nodes will get spread and lost so that when the inner template drop nodes because there's less recursion and the outer template tries to to update, some <div> and some following text node might be gone and it's impossible to have a parent at that point.

I could ignore those cases and just re-append nodes to the persistent fragment reference but that's quirk and dirt ... with arrays, every operation is diffed properly out of a specific pin of nodes, where nodes can be persistent fragments too.

You provide a new persistent fragment, the previous one content will be replaced with the new node without issues, thanks to udomdiff fragment specific logic but the fragment itself won't try, by itself, to remove its nodes that could already be gone.

TL;DR every time content is meant to grow or shrink, the diffing algorithm is the best option everyone have and it's deadly fast too. For code that is never meant to grow or shrink, a hole is all it takes.

This case was meant to grow or shrink, hence I believe using an array there is the right solution/conclusion to fix your issue or you wrap the fragment so that none of this becomes an issue.

I hope I've managed somehow to explain what's going on and why plus how to solve it or change it to make it work best.

WebReflection commented 9 months ago

Latest uhtml fixed the presented tpl use case and more. Passing fragments into fragments is now accepted, as those are handled differently now.

The tpl2 use case though violates the contract that once array always array so that passing [] as fallback is not supported, and likely never will.

What you want to do there is really:

    const tpl2 = (items) => {
        const [first, ...rest] = items
        return html`
            <div>${first}</div>
            ${rest.map(tpl2)}
        `
    }

This preserve the once array, always array contract and doesn't need to deal with a last-entry array nobody would know what to do with.

The former case instead just works now though.

Please read notes in this MR https://github.com/WebReflection/uhtml/pull/105 to understand how the logic works now and expect layout changes as that's inevitable and for good too ... fragments are still not suggested in general, but as these used to work reliably, all I could do was to bring these back in an even better shape.

lazex commented 9 months ago

Thanks for fixing and the detailed explanation.

WebReflection / uhtml

Recursive fragment w/ lost lastChild during target update #102