Recursive fragment w/ lost lastChild during target update #102

lazex · 2024-01-06T02:21:24Z

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 · 2024-01-07T10:49:35Z

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

lazex · 2024-01-07T11:49:39Z

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 · 2024-01-07T12:07:55Z

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 · 2024-01-08T10:09:43Z

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 · 2024-01-08T12:19:34Z

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 · 2024-01-08T14:09:13Z

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 · 2024-01-09T00:58:34Z

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 · 2024-01-09T08:30:30Z

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 · 2024-01-09T08:41:34Z

~~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 · 2024-01-09T12:34:24Z

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 · 2024-01-10T00:25:23Z

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 · 2024-01-11T13:54:59Z

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 · 2024-01-15T19:50:49Z

I did investigate and I am leaning toward this conclusion ... see this comment #103 (comment) 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 · 2024-01-15T20:08:37Z

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.

This MR fixes #102 and fixes #103 + it provides further hydration hints out of the box. Current changes: * each fagment is demilited by `<>` and `</>` comments: see notes * this is a linear render: values are never looped more than once * this version of *uhtml* is even more Memory friendly: a lot has been refactored to consume and recycle as much as possible * the fragment in fragment issue has been resolved * the array hole in tags has been converted into a fragment case * the PersistentFragment has been refactored to survive edge cases * the performance is either better or the same as before * the Array hole now is a `` comment where `N` is the amount of nodes handled * holes are still transparent so that the amount of nodes is still ideal * a new code coverage goal has been reached: 100% of everything, including uhtml/dom * a new test has been written to help out with expectations on the DOM world (browsers) as well as SSR * the SSR story is still to be defined but everything is coming out nicely ... there are fragment hints, array hints, only missing hints to produce a DOM to Template transformer are holes which might land on SSR version only, as it would be ugly to have so many comments in the wild for no reason

WebReflection · 2024-01-18T14:34:09Z

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 #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 · 2024-01-19T17:38:16Z

Thanks for fixing and the detailed explanation.

WebReflection changed the title ~~DOMException: Failed to execute 'setEndAfter' on 'Range': the given Node has no parent.~~ Recursive fragment w/ lost lastChild during target update Jan 9, 2024

WebReflection added the bug Something isn't working label Jan 9, 2024

WebReflection mentioned this issue Jan 18, 2024

Back to the chalk board #105

Merged

WebReflection closed this as completed in #105 Jan 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recursive fragment w/ lost lastChild during target update #102

Recursive fragment w/ lost lastChild during target update #102

lazex commented Jan 6, 2024

WebReflection commented Jan 7, 2024

lazex commented Jan 7, 2024

lazex commented Jan 7, 2024

WebReflection commented Jan 8, 2024 •

edited

lazex commented Jan 8, 2024

WebReflection commented Jan 8, 2024

lazex commented Jan 9, 2024

WebReflection commented Jan 9, 2024

WebReflection commented Jan 9, 2024 •

edited

WebReflection commented Jan 9, 2024

lazex commented Jan 10, 2024

WebReflection commented Jan 11, 2024 •

edited

WebReflection commented Jan 15, 2024

WebReflection commented Jan 15, 2024

WebReflection commented Jan 18, 2024

lazex commented Jan 19, 2024

Recursive fragment w/ lost lastChild during target update #102

Recursive fragment w/ lost lastChild during target update #102

Comments

lazex commented Jan 6, 2024

WebReflection commented Jan 7, 2024

lazex commented Jan 7, 2024

lazex commented Jan 7, 2024

WebReflection commented Jan 8, 2024 • edited

lazex commented Jan 8, 2024

WebReflection commented Jan 8, 2024

lazex commented Jan 9, 2024

WebReflection commented Jan 9, 2024

WebReflection commented Jan 9, 2024 • edited

WebReflection commented Jan 9, 2024

lazex commented Jan 10, 2024

WebReflection commented Jan 11, 2024 • edited

WebReflection commented Jan 15, 2024

WebReflection commented Jan 15, 2024

WebReflection commented Jan 18, 2024

lazex commented Jan 19, 2024

WebReflection commented Jan 8, 2024 •

edited

WebReflection commented Jan 9, 2024 •

edited

WebReflection commented Jan 11, 2024 •

edited