Giter VIP home page Giter VIP logo

Comments (16)

WebReflection avatar WebReflection commented on September 28, 2024 2

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.

from uhtml.

lazex avatar lazex commented on September 28, 2024 1

Thanks for fixing and the detailed explanation.

from uhtml.

WebReflection avatar WebReflection commented on September 28, 2024

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

from uhtml.

lazex avatar lazex commented on September 28, 2024

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.

from uhtml.

lazex avatar lazex commented on September 28, 2024

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/[email protected]/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>

from uhtml.

WebReflection avatar WebReflection commented on September 28, 2024

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?

from uhtml.

lazex avatar lazex commented on September 28, 2024

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'.

from uhtml.

WebReflection avatar WebReflection commented on September 28, 2024

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.

from uhtml.

lazex avatar lazex commented on September 28, 2024

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/[email protected]/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>

from uhtml.

WebReflection avatar WebReflection commented on September 28, 2024

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

from uhtml.

WebReflection avatar WebReflection commented on September 28, 2024

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.

from uhtml.

WebReflection avatar WebReflection commented on September 28, 2024

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.

from uhtml.

lazex avatar lazex commented on September 28, 2024

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.

from uhtml.

WebReflection avatar WebReflection commented on September 28, 2024

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.

from uhtml.

WebReflection avatar WebReflection commented on September 28, 2024

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.

from uhtml.

WebReflection avatar WebReflection commented on September 28, 2024

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.

from uhtml.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.