sveltejs / svelte

web development for the rest of us
https://svelte.dev
MIT License
79.96k stars 4.25k forks source link

The problem with `enumerated` and `Booleanish` #14301

Open AlexRMU opened 2 days ago

AlexRMU commented 2 days ago

Describe the bug

There are several attributes whose values can be enumerated. However, if the values are string representations of booleans, then the value type becomes Booleanish. https://github.com/sveltejs/svelte/blob/320ebd24d8857570b0c180752765fb1580590367/packages/svelte/elements.d.ts#L730 This can lead to similar errors:

Reproduction

https://svelte.dev/playground/98dbee7e667d49028d0d7a662fd58f4e?version=5.1.16

Logs

No response

System Info

-

Severity

annoyance

dummdidumm commented 1 day ago

This looks like an extension of #12664, which #13327 was supposed to fix - but turns out it didn't really, because we forgot about the static case. I'm not exactly sure what's the best way to deal with this is. We have a couple of options:

I'm torn between option b) and c)

AlexRMU commented 1 day ago

Yes, some errors can only be stumbled upon by accident. It's about the nuances of the specification and the operation of browsers. Why are some attributes booleanish and others enumerated? For example, if you replace draggable with contenteditable in the repl, then everything will work, although both of them are enumerated. I think it's better to keep the processing logic simple and choose option c).

dummdidumm commented 1 day ago

Why are some attributes booleanish and others enumerated?

Ask the browser gods, we are as much scratching our heads as you

For example, if you replace draggable with contenteditable in the repl, then everything will work, although both of them are enumerated.

That seems to be a browser quirk where they allow contenteditable="" to mean the same as contenteditable="true", for whatever reason.

AlexRMU commented 1 day ago

I've studied the specification and I think I understand now:

view value
attr ""
attr="" ""
attr="true" "true"
attr="string" "string"

contenteditable = contenteditable="" = "true" draggable = draggable="" = missing/invalid = auto

Leonidaz commented 21 hours ago

This looks like an extension of #12664, which #13327 was supposed to fix - but turns out it didn't really, because we forgot about the static case. I'm not exactly sure what's the best way to deal with this is. We have a couple of options:

  • a) adjust the types: boolean is not allowed anymore, you gotta instead do ="true/false". A breaking change strictly speaking, and doesn't really help when you're not using types. Not a good option IMO
  • b) leave this as is and document it as a caveat, i.e. say that draggable !== draggable={true/false} / {draggable}. One can argue this is correct (since in raw HTML just draggable is also incorrect), but one can also argue that they feel the same.
  • c) make it work by having some special cases for static booleanish attributes that are actually enumerated "true/false"

I'm torn between option b) and c)

I think option c) is probably the best option, if it's not too difficult, since it's highly likely that this is going to keep coming up and would be a "drain" on everyone to deal with submitted issues, including the submitters themselves.

Option b) is likely going to be missed by most in the documentation as it's not something one would be scanning for, and we know how much most of us love carefully reading documentation 😂

Option a) would definitely be way too harsh.

One other option, let's call it d), that would be obvious for devs, but not sure if it's easier than implementing c), is to show a warning if the explicit true / false is missing.