lit / lit

Lit is a simple library for building fast, lightweight web components.
https://lit.dev
BSD 3-Clause "New" or "Revised" License
18.68k stars 919 forks source link

[lit-html] Do not stringify boolean values in child parts #4788

Open maxpatiiuk opened 2 weeks ago

maxpatiiuk commented 2 weeks ago

Should this be an RFC?

Which package is this a feature request for?

Lit Core (lit / lit-html / lit-element / reactive-element)

Description

The following syntax is not supported in lit-html as it will render false to the screen when condition doesn't match:

html`<i>${condition && something}</i>`

lit-html's behavior of stringifying boolean values is not desirable. There is 0 benefit from rendering booleans as "false", which is also what lit docs point out:

A boolean value true will render 'true', and false will render 'false', but rendering a boolean like this is uncommon.

This quirk of lit-html makes migration from Stencil/React/Preact/Vue/Svelte or any other modern framework harder for no good reason - it is a source of bugs in production. Instead, modern rendering libraries ignore falsy values, which reduces the need for bottom values like nothing.

As an extension of https://github.com/lit/lit/issues/1559, I ask you to please reconsider the current behavior of rendering false to the screen.

Alternatives and Workarounds

I wrote a TypeScript transformer that tries to rewrite ${condition && something} into ${condition ? something : nothing} at build time. However, it doesn't catch all the cases, and adds complexity to our build pipeline.

It's no fun to see this in a production app 😓:

Screenshot 2024-10-06 at 12 10 47

justinfagnani commented 1 week ago

Lot doesn't stringify values, it simply passes them to DOM, and the DOM treats them in the standard way. If you do el.textContent = false (which is all Lit the up doing) the DOM will render "false".

We would have to add code and special cases to do different things here, and it would be a breaking change.

maxpatiiuk commented 1 week ago

Thanks for the quick reply!

We would have to add code and special cases to do different things here

There already is code for some falsy values here:

https://github.com/lit/lit/blob/a84771734d99fd8ca17b2257add1acc55b86af1c/packages/lit-html/src/lit-html.ts#L1451

The following might work:

-  if (value === nothing || value == null || value === '') { 
+  if (value === nothing || value == null || value === '' || value === false) { 

or:

-  if (value === nothing || value == null || value === '') { 
+  if (value === nothing || (!value && value !== 0)) { 

and it would be a breaking change.

Are there apps relying on the fact that lit renders false as "false"?

Lookwe69 commented 1 week ago

Personally, it may be problematic to render 'true' but not 'false'. If false value are removed, true must also be removed.

html`${bool}`

Will be inconsistent : 'true' or ''

sorvell commented 1 week ago

Just for comparison, React renders explicit boolean values as empty string. See this example.

sorvell commented 1 week ago

Because this would be a breaking change, we discussed via offline chat potentially rolling the chnage into the when directive if only a single argument is passed.

I think the simplest workaround while that's being considered:

const fmt = (value: any) => typeof value === 'boolean' ? '' : value;

html`${fmt(true)} ${fmt(false)}` // ' '
maxpatiiuk commented 1 week ago

Thanks for the discussion.

My current workaround is to add || "" at build time to any tagged expression like ${a && b}.

true values being rendered is far less of an issue - the main goal is that false is not rendered so that the ${a && b} pattern is usable.

justinfagnani commented 1 week ago

Because this would be a breaking change, we discussed via offline chat potentially rolling the chnage into the when directive if only a single argument is passed.

This should be a simple PR if anyone wants to contribute it.

leogreu commented 4 days ago

This should not be added to the when directive in my opinion, but to the core instead. For me, this issue is the biggest inconvenience of Lit. I have many ternary operators in my code that could be simplified if false (or boolean) wouldn't be rendered. I've never encountered the need to render boolean values directly to the DOM.

The following syntax is so much more concise and elegant:

${condition && html`
    <custom-element></custom-element>
`}

... than this:

${condition
    ? html`
        <custom-element></custom-element>
    `
    : undefined
}

Having this functionality built into when would require an import for every single conditional render – and it wouldn't simplify the syntax either. Lastly, I would consider not rendering false or boolean values as industry standard since JSX and other languages do the same.

justinfagnani commented 4 days ago

@leogreu it would be a breaking change for lit-html to not render boolean values.

leogreu commented 4 days ago

@justinfagnani Yes, unfortunately. But since you tagged it with 4.0, I thought it would make sense to introduce it as a breaking change rather than adding it to when.

maxpatiiuk commented 4 days ago

Agree with @leogreu - I do not see the benefit of adding this to when.

justinfagnani commented 4 days ago

The only benefit of any of this is shorthand. Ternaries work today, but are deemed cumbersome, but some.

when(condition && html`<custom-element></custom-element>`)

is slightly shorter than

condition ? html`<custom-element></custom-element>` : undefined

that's all.

Also, I'm not even sure that we want this breaking change in 4.0. To me, it makes total sense to render booleans, just like every other value, including other primitives like numbers.

html`The data is valid: ${isValid}`

Having to do:

html`The data is valid: ${String(isValid)}`

but only for booleans is an inconsistency I'd much rather not have, and not have to explain.

I don't personally consider React doing something to be a automatic reason to also do it.

leogreu commented 4 days ago

I don't personally consider React doing something to be a automatic reason to also do it.

I agree with that. But its not only about React, its about JSX in general (used for example in Stencil and Astro as well) and Angular (not sure about current Vue and Svelte) which doesn't render false.

The only benefit of any of this is shorthand. Ternaries work today, but are deemed cumbersome, but some.

when(condition && html<custom-element></custom-element>) is slightly shorter than

condition ? html<custom-element></custom-element> : undefined that's all.

If you have more attributes that you want to pass to your custom-element, it looks differently. You have another level of indentation when using a ternary operator, which sums up and makes the code more verbose and difficult to read. Currently, when requires a function for the trueCase, making it lengthier as well (plus the required import).

Also, I'm not even sure that we want this breaking change in 4.0. To me, it makes total sense to render booleans, just like every other value, including other primitives like numbers.

htmlThe data is valid: ${isValid} Having to do:

htmlThe data is valid: ${String(isValid)} but only for booleans is an inconsistency I'd much rather not have, and not have to explain.

I agree that consistency is important, but you don't render undefined and null which are primitives, too. I think adding boolean to that list would make sense. Usually you don't want to render true or false to your users. You want to render Yes and No, Active or Inactive, etc. and of course numbers or other strings.

justinfagnani commented 4 days ago

I agree that consistency is important, but you don't render undefined and null which are primitives, too.

I was personally against that for this very slippery slope reason. I like to match what the DOM does in these cases.

sorvell commented 3 days ago

The following syntax is so much more concise and elegant

I'm pretty sympathetic to this. These types of conditions are very common and this makes the simpler syntax extremely valuable.

A workaround you can do today:

import {html as litHtml} from 'lit';
const html = (strings, ...values) => litHtml(strings, ...values.map(v => typeof v === 'boolean' ? '' : v));

With minimal cost, we could introduce something to renderOptions to optionally add this behavior. This could be a general purpose value thunk or targeted exactly to boolean rendering:

render(html`${condition && 'hi'}`, node, {noBooleanRendering: true})
leogreu commented 3 days ago

The idea with renderOptions sounds great! It wouldn‘t be a breaking change anymore, it would maintain portability since its defined per component, and it allows inheritance (and thus to define it once in a custom base class and reuse it across an app or design system).