solidjs / solid

A declarative, efficient, and flexible JavaScript library for building user interfaces.
https://solidjs.com
MIT License
32.4k stars 924 forks source link

Boolean attributes #1101

Closed edemaine closed 2 years ago

edemaine commented 2 years ago

Questions: How should JSX attributes foo and foo={true} behave for an HTML element, as in <div foo> or <div foo={true}>? Should the behavior differ for components and/or spreads?

Current behavior in Solid: Assume const Div = (props) => <div {...props}/>

JSX Equivalent HTML
<div foo> <div foo="">
<div foo=""> <div foo="">
<div foo={true}> <div foo="true">
<div foo="true"> <div foo="true">
<Div foo> <div foo="true">
<Div foo=""> <div foo="">
<Div foo={true}> <div foo="true">
<Div foo="true"> <div foo="true">

The ☆ behavior (another consequence of setAttribute's string casting) is particularly counterintuitive, as it differs between HTML elements and component-wrapped elements. (It may also be an SSR discrepancy?) @fabiospampinato's observation of this weirdness is what spawned this exploration (in #templating channel, also with @LXSMNSYC).

Relevant facts:

Desired properties:

  1. foo is always equivalent to foo="" for HTML/SVG elements (as in HTML 5). This is useful for copy/pastability of HTML, which the JSX spec considers important.
  2. foo is equivalent to foo={true} (as in React and TypeScript)
  3. draggable={true} is equivalent to draggable="true"
  4. <div foo> is equivalent to const Div = (props) => <div {...props}/>; <Div foo>

We can't have all four properties.

Proposal: I really like Property 1, and would propose gaining Property 4 by compiling foo={value} to essentially _el$.setAttribute('foo', value === true ? '' : value), where value is stored in a temporary variable if it's a general expression, with a similar modification to spreads. This would basically flip ☆, and also change those marked "!":

JSX Equivalent HTML
<div foo> <div foo="">
<div foo=""> <div foo="">
<div foo={true}> <div foo=""> !
<div foo="true"> <div foo="true">
<Div foo> <div foo="">
<Div foo=""> <div foo="">
<Div foo={true}> <div foo=""> !
<Div foo="true"> <div foo="true">

(By contrast, React's approach of an attribute whitelist feels gross...)

fabiospampinato commented 2 years ago

I think potentially there's a 5th desired property:

Maybe this isn't super desirable or it's too much of an edge case, but since using Solid with React's transform is supported I'd find it strange if something completely static like <div draggable /> produced different divs depending on which transform is used, though in general some differences are expected or the custom transform wouldn't exist.

otonashixav commented 2 years ago

To be clear, this wouldn't affect directives, correct? i.e. <div use:foo> still calls foo(ref, () => true), and not foo(ref, () => "").

LiQuidProQuo commented 2 years ago

what about the case where you want to remove the attribute undefined probably false null maybe

edemaine commented 2 years ago

To be clear, this wouldn't affect directives, correct? i.e. <div use:foo> still calls foo(ref, () => true), and not foo(ref, () => "").

Correct. It won't even affect props in components: foo will still appear as props.foo === true. It's just about when something hits the DOM, which never happens for directives.

what about the case where you want to remove the attribute undefined probably false null maybe

undefined and null already behave this way (whereas false renders as "false"). Oops, I didn't realize there's already a setAttribute helper which could be used for exactly this purpose:

https://github.com/ryansolid/dom-expressions/blob/3c593d43147a73ddd52c58ba8c49f4f1e0e7e560/packages/dom-expressions/src/client.js#L68-L71

I took a stab at implementing the proposal here: https://github.com/ryansolid/dom-expressions/pull/141

LiQuidProQuo commented 2 years ago

undefined and null already behave this way

yes

(whereas false renders as "false").

but then isn't odd if true does this: <div foo={true}> | <div foo=""> and false does this <div foo={false}> | <div foo="false">

?

ryansolid commented 2 years ago

I see. In general we set known boolean attributes by property and everything else by setAttribute. I see I left boolean(no arg) working like a boolean in it literally sets empty string but this is probably wrong. A boolean attribute cannot be removed so there is no inconsistency on that side but it means we probably should be setting it to true.

We used to do this the other way but there are attributes that actually want "true" and "false". I think we should continue checking against the list and apply it to the boolean form. I guess thats siding with React. But the other way is no good either.

There is no guarantee it ends up in the DOM through a component so it has to come in as true. The spread has the choice. And we've landed on pass list. So i think it has to be the first example that is off.

EDIT: If you are wondering the source of Solid's implementation. As usual Inferno is what I trust for these sort of things. React probably has the most thorough but I suspect this is a place both Inferno and Preact save size by doing something reasonable.

lxsmnsyc commented 2 years ago

Some helpful resource (from React).

Nothing is mentioned about aria-* properties, but should we handle them like draggable?

Edit: Seems like Solid already properly handles aria-* properties based on my tests on solid-headless

edemaine commented 2 years ago

This is a great analysis of React behavior @LXSMNSYC! I think this line actually makes aria-* and data-* behave like aria-*="true" and data-*="true"; at least that matches my testing.

Obviously one option is to reproduce React's behavior. I think there's still an open question of what to do with attributes that aren't on any of these lists:

  1. We could default to foo/foo={true} meaning foo="true", as Solid currently does.
  2. We could default to foo/foo={true} meaning foo="" as in Desired Property 1 / HTML 5 spec.
  3. We could default to foo/foo={true} meaning "make no attribute, but issue warning", which I just discovered React does.

Personally I think Option 2 makes sense, given HTML spec, but it differs from both current Solid and React. An advantage of this approach is that you only need a list of "enumerated but Boolean-like" attributes, like those listed in bullets 1 and 2 of @LXSMNSYC's post and aria-* (not sure about React's decision to include data-* — that feels like it should be application-specific, so matching HTML behavior makes more sense to me). No need to have a list of all Boolean properties because Option 2 will just handle them correctly. (There's a similar advantage to Option 1: you'd "only" need to list Boolean properties, not pseudo-Boolean properties. But I think that list is longer.)

lxsmnsyc commented 2 years ago

With data-*, how does dataset property work with raw boolean values?

edemaine commented 2 years ago

With data-*, how does dataset property work with raw boolean values?

Good point: dataset casts true to "true". Currently, Solid seems to use setAttribute for setting data-* attributes, but if it switched to using dataset in the future, it might make sense to map data-foo to data-foo="true" for consistency with the reactive case.

ryansolid commented 2 years ago

If I didn't make it clear in my previous response. While I see potential of changing more things and doing stuff differently. It's the first row that is the bug today and should be fixed. We already default to true except for a pass-list and I'm not prepared to change that at the moment. We were the other way in the past but it caused other confusion and errors.

So I've updated this case to use that pass-list: <div foo> => <div foo="true">. Where a known boolean attribute would work appropriately.

edemaine commented 2 years ago

For what it's worth, I recently bumped into this React GitHub issue where users were confused about the inconsistent behavior, in particular for data-* attributes and how it differs from HTML. (They write "JavaScript" but they mean the innerHTML interface.)

https://github.com/facebook/react/issues/24812

At the least I think we need to document things clearly here. (Personally I'd also much rather have the default be "like HTML" instead of "like React".)

lxsmnsyc commented 2 years ago

I think we should follow the convention of handling booleans for specific attributes, just for consistency. It's just a small price to pay.

LiQuidProQuo commented 2 years ago

but this generalization works for any known and unknown boolean attribute ( will passing "true" to a known html attribute be considered non standard? will it not work as expected?)

it is also more or less consistent between Component and an Element. even if slightly deviates from native html interpretation foo=""

Element

<div foo> === <div foo={true}> => <div foo="true">

Component

<MyComp foo/>
const MyComp = function(p){  
console.log(p.foo===true) // true 
...
 }

null value

null, is the a special case, as it removes the attribute, and doesn't simply assign the value natively ( for Element, for component value is passed)

<div foo={null}> => <div>

and if one doesn't like the "true" they can be explicit and set <div foo="" />

ryansolid commented 2 years ago

This thread is relevant here: https://github.com/solidjs/solid/issues/383. Where I justified the current behavior at the end by being browser. But I didn't realize the inconsistency with spread. It looks like I pulled inspiration from Svelte for the current behavior but did not test spread there I assume.

I do think coercing {true} to "" and {false} to remove can cause issues which means changing this feels major version to me. It is interesting though. See we used to set all things as properties so boolean would just work that way. Then we changed to default to attributes which meant that I'd need to look for falsey to remove the attribute. I kept booleans on elements being "" simply because it was simple enough to do and I justified it by being like the browser. But I suppose if we really wanted to be like the browser we would force people to use "true" for draggable. Or their webcomponents that expect it not to be handled like a boolean attribute. This is a defensible position even if I find it unfortunate. But masking browser behavior is awkward. Generally when in doubt we do side with the browser, so this is strangely inconsistent with that.

That being said from my previous research not a single other JavaScript framework aligns with the browser here. true means "true" and false means "false" with exception of the pass-list. Even close to the browser frameworks like Preact or Svelte. Preact works identical to how I proposed to fix this. And Svelte works exactly like how Solid works today with the inconsistency on the spread.

edemaine commented 2 years ago

That thread is a neat read. The idea of bool:foo={true} meaning foo="" is an interesting approach.

It's a shame that browser and frameworks are so divergent here. I still like siding with the browser default, for the sake of "you can copy/paste HTML and it just works the same".

But I suppose in actual practice, it doesn't really make much difference as long as you stick to standard attributes, so either way you'll get whitelisted to Boolean or pseudo-Boolean behavior as needed. The main practical difference you'd see is probably data attributes, where <div data-foo> differs between browser and React or Solid (under Ryan's proposal, unless we decide to whitelist data-* as Boolean too).

ryansolid commented 2 years ago

I think the one thing that we have going for us is that foo={true} isn't a copy-paste. Like the decision of how we handle booleans is independent of this until we consider spreads. But components are also not copy-paste. So if we left everything exactly as it is, like Svelte, other than the expectation on spreading boolean attributes coming from a component which is arguably a bit whatever we are retaining copy-pasteability.

It's possible the right answer is no change.

LiQuidProQuo commented 2 years ago

no change means

      <button draggable={true}>can drag</button>
      <button draggable>can't drag</button>
ryansolid commented 2 years ago

@LiQuidProQuo

Yes which came up with the linked issue. I have very little problem with this.

The argument is the browser doesn't work this way either. We are free to define the meaning of draggable={true} but draggable as copy pasted from StackOverflow will not work all the same without being given a true value. The current behavior doesn't violate the browser at all but leaves us free to have the convenience of using booleans instead of always using strings for these sort of things.

I guess I'm curious would you rather this be the case or that neither worked and you needed to do:

<button draggable={condition() ? "true" : "false"}>can drag</button>

I'm trying to gauge the cost of compromise here. Like is the concern that these look inconsistent? Or that it requires the knowledge of how draggable and boolean attributes work.

LiQuidProQuo commented 2 years ago

@ryansolid

honestly I am just realizing that draggable is an edge case, it is not really a boolean attribute but a boolean value based.

so if we look at a real boolean attribute(presence base), we need to get to the equivalent of: <input type="checkbox" checked={condition() ? "checked" : null}>is checked?</button> or <input type="checkbox" checked={condition() ? "" : null}>is checked?</button> which is already cover by current behavior

"" | "checked" is to comply with the recommendation of the standard, but "true" also works

I am not personally in favor of making draggable get treated as an edge case(not sure if there are other attributes like it),

I did like your proposed change, as it seem to strike a good balance. <div foo> => <div foo="true"> it "works/normalize" with fake boolean attribute draggable or any real one.

but realizing that draggable is an exception, I am thinking no change might actually be better users of draggable will just have to be explicit, like they need to in actual html

<button draggable={true} />

but then for Components do we expect prop.draggable true or undefined ? <Button draggable /> I prefer that true is passed in props, because it simplifies the check, compared to checking if key exists in the props.

so perhaps I am also starting to lean towards no change

and any special edge case magic, should have special dedicated explicit syntax in my opinion( if its worth having).

edemaine commented 2 years ago

I'd indeed prefer "no change" over changing the meaning of <div foo>; this indeed preserves copy/pastability.

But could we change spread to match, i.e., change the behavior of ☆ in the original post? Here's a perhaps better illustration of the inconsistency. I guess it's only "inconsistent" if you think of foo and foo={true} as equivalent, which perhaps they only are in the presence of components...

ryansolid commented 2 years ago

I'm not sure it is possible to do just that. Because at that point all we know is foo = true so all spread can do is treat it the same. And boolean attribute foo on a Component I think has to be true. So really only 2 options. I guess technically 3. React/Preact/Vue/Inferno, Svelte/Solid, or treat all booleans as boolean attributes by default.

ryansolid commented 2 years ago

Yeah I think Svelte is good company here. Let's leave this as is with one exception. I was handling it the Preact/React way in SSR. I will change that and hopefully that will be consistent enough. Dynamic probably needs to be part of the compiler. I'm not sure exactly what that looks like but I think that is the only way we deal with some of the desired gaps.

ryansolid commented 2 years ago

Closing with 1.5 release.

trusktr commented 5 months ago

Continued in