Inconsistency in SSR boolean and null attributes when using spread syntax

[domingues]

<input type="text" disabled={true}/>
<input type="text" {...{disabled:true}}/>
<input type="text" disabled={1}/>
<input type="text" {...{disabled:1}}/>
<input type="text" value={null}/>
<input type="text" {...{value:null}}/>

will render

<input type="text" disabled>
<input type="text" disabled disabled="true">
<input type="text" disabled>
<input type="text" disabled="1">
<input type="text">
<input type="text" value="null">

The duplication on line two could also be seen in REPL with:

<script>
	import { spread } from "svelte/internal"
</script>
{spread([{disabled:true}])}

[Conduitry]

On latest master (3.13.0-alpha.2) I'm currently getting

<input type="text" disabled>
<input type="text" disabled disabled="true">
<input type="text" disabled>
<input type="text" disabled="1">
<input type="text">
<input type="text">

So spreading objects with nulls in them has been fixed (by #3775).

I'm undecided whether {...{disabled:1}} rendering as disabled="1" is actually a problem. It would still be nice to get that to be just disabled.

{...{disabled:true}} rendering as disabled disabled="true" is definitely a bug and is weird, and I will look into that soon.

[Conduitry]

The double attribute issue is just a simple logic error, and can be fixed with

diff --git a/src/runtime/internal/ssr.ts b/src/runtime/internal/ssr.ts
index 83e585a8..61f81016 100644
--- a/src/runtime/internal/ssr.ts
+++ b/src/runtime/internal/ssr.ts
@@ -21,7 +21,10 @@ export function spread(args, classes_to_add) {
 
 		const value = attributes[name];
 		if (value == null) return;
-		if (value === true) str += " " + name;
+		if (value === true) {
+			str += " " + name;
+			return;
+		}
 
 		const escaped = String(value)
 			.replace(/"/g, '"')

It's not specifically mentioned in this issue, but I'm not sure how we'd want to deal with something like <input {...{ disabled: false }}>. Right now, this renders <input disabled="false">, which is incorrect. It seems the only way to handle this correctly is to ship the array of boolean attributes as a helper used by SSR. It doesn't seem avoidable, as there's no way to partially rely on the DOM API in SSR mode. I'd been really against including these sorts of tables of metadata in compiled DOM mode code, but including it in SSR mode code doesn't seem so bad.

[Conduitry]

Pushed what I have so far to this branch. One slight wrinkle is just that this introduces code that we want to have available at build time and at run time, which I don't think has precedent. I elected to stick this shared data in src/compiler/compile/render_ssr/handlers/shared/boolean_attributes.ts, and then to also import it from there in src/runtime/internal/ssr.ts. TypeScript and ESLint don't seem to have any complaints about that.

There are also still some differences between DOM and SSR mode in how non-boolean attributes that are explicitly given foo={true} values (as opposed to simply being bare attributes) are treated - but these differences exist even without spreads involved, and it's really an edge case, so I don't think it's something that we need to worry about here.

Another difference between DOM and SSR handling of boolean attributes is that in SSR mode, no checking is done to see whether this attribute is boolean for this element. There's just a list of boolean attributes. We can revisit this later if it becomes a problem, but in the meantime it would probably be prudent to update the list of boolean attributes according to the attribute_lookup fleshing out that was done a few months ago - both the addition and the removal of attributes that are considered boolean. I'll do this shortly.

[Conduitry]

So, as with a couple of these spread-related issues, the scope of my in-progress PR ends up expanding beyond the original issue, oh well.

Something that currently generates incorrect code in SSR mode is this:

<script>
	let autocomplete = 'no';
</script>

<input {autocomplete}>

Since autocomplete is (incorrectly) currently seen as a boolean attribute, it would be conditionally attached depending on whether foo is truthy. With this branch, it will be treated as a normal attribute, which it is.