I've added a runtime error that checks if the html generated by thetemplate is the same structure as the passed in string. This should prevent more obscure errors because if the structure is different our marching of the dom will still fail at runtime.

To properly check i had to transform both the innerHTML and the incoming structure to remove inner texts (that get's converted in html entities) and attributes (because boolean attributes get="" appended automatically.

Also i don't do this check in{@html} because you could technically pass invalid html there but that doesn't affect the marching algorithm.

Another idea I had (might still open a PR as an alternative) is to add avalidate_element call after everynext orchild...basically like this

import'svelte/internal/disclose-version';import'svelte/internal/flags/legacy';import*as$from'svelte/internal/client';varroot=$.template(`<p></p> <tr><td></td></tr>`,1);exportdefaultfunctionApp($$anchor){letname='world';varfragment=root();vartr=$.sibling($.first_child(fragment),2);$.validate_element(tr,'tr');vartd=$.child(tr);$.validate_element(td,'td');td.textContent=name;$.reset(tr);$.append($$anchor,fragment);}

but i think this would be more unreliable.

Before submitting the PR, please make sure you do the following

It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC:https://github.com/sveltejs/rfcs
Prefix your PR title withfeat:,fix:,chore:, ordocs:.
This message body should clearly illustrate what problems it solves.
Ideally, include a test that fails without this PR but passes with it.
If this PR changes code withinpackages/svelte/src, add a changeset (npx changeset).

Tests and linting

Run the tests withpnpm test and lint the project withpnpm lint

fix: throw runtime error when template returns different html

a9b26c3

Copy link

changeset-botbot commentedMar 16, 2025•
edited
Loading

🦋 Changeset detected

Latest commit:62a6a15

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
svelte	Patch

Not sure what this means?Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

svelte-docs-botbot commentedMar 16, 2025

Preview:https://svelte-dev-git-preview-svelte-15524-svelte.vercel.app/

Copy link

Contributor

github-actionsbot commentedMar 16, 2025

Playground

pnpm add https://pkg.pr.new/svelte@15524

github-advanced-securitybot found potential problems

Mar 16, 2025

View reviewed changes

packages/svelte/src/internal/client/dom/reconciler.js DismissedShow dismissedHide dismissed

Rich-Harris reviewed

Mar 17, 2025

View reviewed changes

Copy link

Member

Rich-Harris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Nice idea! This would be a breaking change though so it would need to be a warning rather than an error. I suspect the proposed alternative approach would work better — details inline. (It wouldn't work for{@html ...} though... so maybe we need both?)

Also, we should do this checking eagerly, otherwise no warning will be issued for malformed HTML in a block that happens not to get rendered

packages/svelte/src/internal/client/dom/blocks/html.js Outdated

		@@ -99,7 +99,7 @@ export function html(node, get_value, svg, mathml, skip_warning) {
		// Don't use create_fragment_with_script_from_html here because that would mean script tags are executed.
		// @html is basically `.innerHTML = ...` and that doesn't execute scripts either due to security reasons.
		/** @type {DocumentFragment \| Element} */
		var node = create_fragment_from_html(html);
		var node = create_fragment_from_html(html, false);

Copy link

Member

Rich-HarrisMar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

can you elaborate on why we're not doing this for{@html ...}? It may survive hydration but it's still delivering an unexpected outcome. Feels like we should just do it in all cases

Copy link

MemberAuthor

paoloricciutiMar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It mainly goes down to self-closing-but-not-really tags.

{@html"<div />"}

This will yield an error (I suppose if we switch to a warning it could be fine tho)

packages/svelte/src/internal/client/dom/reconciler.js Outdated

		// we remove the text within the elements because the template change & to & (and similar)
		.replace(/>([^<>]*)/g, '>');
		if (remove_attributes_and_text_input !== remove_attributes_and_text_output) {
		e.invalid_html_structure(remove_attributes_and_text_input, remove_attributes_and_text_output);

Copy link

Member

Rich-HarrisMar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The HTML snippets could become arbitrarily large (especially if we enabled this for{@html ...} as well), we may need to adding truncation here, or remove common prefixes/suffixes etc

The alternative approach of checking the structure manually could help here since it means we can point to the specific element that's being mishandled. (Perhapsadd_locations becomes a validation function as well — it already knows the intended structure.)

Copy link

MemberAuthor

paoloricciutiMar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yeah I though about that but also thought a diffing algorithm would be too much and not showing complete information would be not really helpful.

I can try to put up a PR for the alternative method tomorrow to check if it could yield better results

Copy link

MemberAuthor

paoloricciuti commentedMar 17, 2025

This would be a breaking change though so it would need to be a warning rather than an error

Can you think of some cases where having the wrong output from the template would not lead to a runtime error anyway? I thought of making it a warning but I also thought that it would be a runtime error anyway.

Copy link

Member

Rich-Harris commentedMar 17, 2025

Many cases where you're not hydrating (the test case in this PR, for example). Hopefully there aren't too many of those cases in the wild but there's probably some where it's 'failing' in a way that's harmless

Copy link

MemberAuthor

paoloricciuti commentedMar 17, 2025

Many cases where you're not hydrating (the test case in this PR, for example). Hopefully there aren't too many of those cases in the wild but there's probably some where it's 'failing' in a way that's harmless

Mmm that's annoying...well gonna move it to warning tomorrow 😁

fix: move the error to a warning

62a6a15

Copy link

Contributor

7nik commentedMar 18, 2025

Do I miss something orthe issue example still throws the same error? And I don't see how it will help when the problem caused by invalid composition of valid components, e.g.<Button><Button>btn</Button></Button>

Copy link

MemberAuthor

paoloricciuti commentedMar 18, 2025

Do I miss something orthe issue example still throws the same error? And I don't see how it will help when the problem caused by invalid composition of valid components, e.g.<Button><Button>btn</Button></Button>

Yes because now it's just a warning that you can see in the console.

Also no, it will not solve that issue, only local marching problems

Copy link

Member

Rich-Harris commentedMar 18, 2025

Come to think of it, couldn't we find these issues at compile time?

Copy link

Member

Rich-Harris commentedMar 18, 2025

(I mean not for{@html ...} but for anything that gets turned into a$.template call)

Copy link

MemberAuthor

paoloricciuti commentedMar 18, 2025

Come to think of it, couldn't we find these issues at compile time?

The main issue i see with this is that I'm pretty sure this is browser dependent...a browser might correct in a different way. Also keeping up with the actual transformation the browser does would be very error prone.

Copy link

Member

Rich-Harris commentedMar 18, 2025

I would bevery surprised if browsers differed — this stuff is all spec'd, it's not open to interpretation. I'm not sure exactly what rules we'd need to follow but I bet we could figure it out

Copy link

Member

dummdidumm commentedMar 28, 2025

Come to think of it, couldn't we find these issues at compile time?

We already find all statically analyzable issues at compile time. But once the invalid-ness of the template appears cross-component you can only check it at runtime.

Copy link

Member

Rich-Harris commentedMar 31, 2025

The test case in this PR contains a single component:

<p></p><tr></tr>

So we'renot finding all statically analyzable issues at compile time. And this PR doesn't touch cross-component stuff, it only warns on invalid templates. Aside from{@html ...}, there's nothing we can detect at runtime that we couldn't in theory detect at compile time where it's much more actionable. So I don't think we should merge this in its current state — converting to draft for now

Rich-Harris marked this pull request as draft

March 31, 2025 19:35

Copy link

MemberAuthor

paoloricciuti commentedMar 31, 2025

I think we would have to do something similar to what Rich is suggesting to support the new quirky chrome parsing of select/option so we might just as well fix it in a more general way when we also fix that bug

Labels

None yet

4 participants

Movatterモバイル変換

Uh oh!

fix: throw runtime error when template returns different html#15524

Are you sure you want to change the base?

fix: throw runtime error when template returns different html#15524

Uh oh!

Conversation

paoloricciuti commentedMar 16, 2025

Before submitting the PR, please make sure you do the following

Tests and linting

Uh oh!

changeset-botbot commentedMar 16, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

svelte-docs-botbot commentedMar 16, 2025

Uh oh!

github-actionsbot commentedMar 16, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Rich-Harris left a comment

Choose a reason for hiding this comment

Uh oh!

Rich-HarrisMar 17, 2025

Choose a reason for hiding this comment

Uh oh!

paoloricciutiMar 17, 2025

Choose a reason for hiding this comment

Uh oh!

Rich-HarrisMar 17, 2025

Choose a reason for hiding this comment

Uh oh!

paoloricciutiMar 17, 2025

Choose a reason for hiding this comment

Uh oh!

paoloricciuti commentedMar 17, 2025

Uh oh!

Rich-Harris commentedMar 17, 2025

Uh oh!

paoloricciuti commentedMar 17, 2025

Uh oh!

7nik commentedMar 18, 2025

Uh oh!

paoloricciuti commentedMar 18, 2025

Uh oh!

Rich-Harris commentedMar 18, 2025

Uh oh!

Rich-Harris commentedMar 18, 2025

Uh oh!

paoloricciuti commentedMar 18, 2025

Uh oh!

Rich-Harris commentedMar 18, 2025

Uh oh!

dummdidumm commentedMar 28, 2025

Uh oh!

Rich-Harris commentedMar 31, 2025

Uh oh!

paoloricciuti commentedMar 31, 2025

Uh oh!

Uh oh!

changeset-botbot commentedMar 16, 2025•
edited
Loading