Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Svelte 5: Nested form control bindings causes runtime error #11941

Open
Tracked by #11400
Hugos68 opened this issue Jun 6, 2024 · 10 comments · Fixed by #11947
Open
Tracked by #11400

Svelte 5: Nested form control bindings causes runtime error #11941

Hugos68 opened this issue Jun 6, 2024 · 10 comments · Fixed by #11947
Milestone

Comments

@Hugos68
Copy link

Hugos68 commented Jun 6, 2024

Describe the bug

When binding to inputs in nested forms, the binding fails and a runtime error is thrown:

<form>
	<form>
			<input bind:value />
	</form>
</form>

Although nested forms are not valid according to the HTML spec, the error should be either friendlier or not throw at all since it worked just fine in Svelte 4.

Reproduction

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAACkWM2wrDIBBEf0WWQloQfLe20O-ofbDJCoI3dA2EkH8vNoEwL2eGw6xgnccK8r1CNAFBwitn4EBL7qXO6AmBQ02tjH1RdSwu01NHTR6JzcY3ZA92qWQIr8Nwu-uoxGlFZVMJf_-kHuVibsS-Lk5yfxG7JQ7tAOAQ0uSswwkklYbbZ_sBkmW8mbgAAAA=

Logs

Cannot read properties of null (reading 'addEventListener')

in App.svelte

System Info

running Svelte compiler version 5.0.0-next.151

Severity

blocking an upgrade

@dummdidumm
Copy link
Member

This is listed as one of the breaking changes in Svelte 5. What's your use case for creating invalid HTML?

@Conduitry
Copy link
Member

The error here isn't actually a compiler error - it's a runtime error - but, in cases where we can see both <form>s in one component, maybe we could add an actual compiler error for this, like we do for other types of invalid element nesting.

@Hugos68
Copy link
Author

Hugos68 commented Jun 6, 2024

This is listed as one of the breaking changes in Svelte 5. What's your use case for creating invalid HTML?

No use case, just porting Svelte 4 code, this made us realize we were actually shipping invalid HTML, so it's kind of good this errors then. It was a bit hard to spot since one form lived deeply inside a component. If this is as expected you may close this, could you link where this is listed as a breaking change?

I just think it's a bit hard to know what you're doing wrong since it only throws when binding inside a nested form but not without. Could be confusing idk.

@Hugos68 Hugos68 changed the title Svelte 5: Nested form control bindings causes compiler error Svelte 5: Nested form control bindings causes runtime error Jun 6, 2024
@dummdidumm
Copy link
Member

dummdidumm commented Jun 6, 2024

Huh I tell a lie, this isn't listed yet in the breaking changes.
Therefore giving it the documentation label.

@paoloricciuti
Copy link
Member

The error here isn't actually a compiler error - it's a runtime error - but, in cases where we can see both <form>s in one component, maybe we could add an actual compiler error for this, like we do for other types of invalid element nesting.

I agree with this...if it's statically analizable throwing a compiler error with a nice message is preferrable.

I can work on this but i would love some kind of approval if that's something we actually want to do.

@dummdidumm
Copy link
Member

dummdidumm commented Jun 6, 2024

Yes, we already have a validation for this in place, seems like it's missing the form/form case. Though as others have said this only works when things are within the same component. In the mid term it would be cool to investigate if there's something we can do at runtime in dev mode

@paoloricciuti
Copy link
Member

Yes, we already have a validation for this in place, seems like it's missing the form/form case. Though as others have said this only works when things are within the same component. In the mid term it would be cool to investigate if there's something we can do at runtime in dev mode

will check on that :)

@paoloricciuti
Copy link
Member

Yes, we already have a validation for this in place, seems like it's missing the form/form case. Though as others have said this only works when things are within the same component. In the mid term it would be cool to investigate if there's something we can do at runtime in dev mode

So i've started investingating this a bit:

  1. the general problem is that by creating the html using <template /> any invalid HTML will be attempted to be corrected, in this case is eliminating the outer form causing the code of the component to be off by one
import * as $ from "svelte/internal/client";

var root = $.template(`<form><form><input></form></form>`); // this ends like this <form><input></form>

export default function App($$anchor) {
	let value = $.source('');
	var form = root();
	var form_1 = $.child(form); // the child of the form is the actual input
	var input = $.child(form_1); // the input doesn't have a child so this is null

	$.remove_input_attr_defaults(input);
        // this fails because is setting a listener on null is not possible.
	$.bind_value(input, () => $.get(value), ($$value) => $.set(value, $$value)); 
	$.append($$anchor, form);
}
  1. the whole bug will be obviously fixed by the validator but cases like this might happen anyway for other wrong HTML which we are not yet aware of. The situation can get also much worst if you don't add listeners on the input but on the inner form because in that case you are actually adding everything to the input. The good news is that technically this is "fixable" statically because is you stick the form in a component it get's it's own template root and just ships the double form to the browser (not ideal but at least doesn't fail cryptically).
  2. one possible idea for this could be to always validate the output of a function to retrieve an element checking against the tagName
import * as $ from "svelte/internal/client";

var on_input = () => {};
var root = $.template(`<form><form><input></form></form>`);

export default function App($$anchor) {
	var form = root();
        $.validate_element(form, "form");
	var form_1 = $.child(form);
        $.validate_element(form_1, "form");
	var input = $.child(form_1);
        $.validate_element(input, "input");

	input.__input = [on_input];
	$.append($$anchor, form);
}

$.delegate(["input"]);

this in theory is guaranteed to error for at least 1 element (either is null or the wrong expected element) but to be fair i don't know how well it would scale and the performance hit.

In the meantime i'm gonna open the PR to fix the validator we can iterate there or in another PR.

@Rich-Harris
Copy link
Member

Reopening this since we still haven't fixed <form><Form></Form></form>, which makes hydration errors very likely. Opened #11969 to investigate further

@Rich-Harris Rich-Harris reopened this Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants