[core] Use boolean type for ListItem prop instead of 'false'

[ricosandyca]

Sometimes, we might need to assign button type as boolean. Last time i tried conditional variable passed to button prop. Something like this

<ListItem button={!!item.onClick} />

But i got an error type caused type boolean isn't assignable to type true or false

• I have followed (at least) the PR section of the contributing guide.

For #14971

[mui-pr-bot]

No bundle size changes

Generated by 🚫 dangerJS against 09c897f

[eps1lon]

This can be considered better than before but <ListItem button={someBooleanVal} /> will never accept button props. It'll always be considered a li and I'm not sure what's better: failing loudly with "boolean really just doesn't work" or failing silently and complaining later on unassignable props. We're basically kicking type errors down the stack.

[ricosandyca]

@eps1lon you could still accept the button props if you assign the value of button directly as true

<ListItem
  button
  onClick={e => ....} // `e` variable has `HTMLDivElement` type
/>
<ListItem
  button={boolValue} // or `false`
  onClick={e => ....} // `e` variable has `HTMLLIElement` type
/>

That's why i changed the type order of true value of button as the first checking

[eps1lon]

<ListItem
button={boolValue} // or false
onClick={e => ....} // e variable has HTMLLIElement type
/>

What if boolValue is true at runtime? The typings expect that boolean is the same as false which is not sound. I'm not fully convined that your approach is better. It just seems to hide issues.

[ricosandyca]

But not always the same. The reasons why ListItem read as div component, because it receive true exact value. Otherwise, if ListItem receive boolean variable which is it could be true or false it will be read as the default component that is li.

So, if you want get some div component types suggestion, you have to assign the button prop as true directly.

I think this is better, cz it can assign the button variable dynamically. Unlike the old one that can only receive static value

[eps1lon]

I think this is better, cz it can assign the button variable dynamically. Unlike the old one that can only receive static value

You're trading soundness. Whether that is "better" depends on how sound you want your types to be. You could achieve the same right now by passing <ListItem button={boolValue as false} />. With that you're directly aware of potential problems. With your change you ain't.

[oliviertassinari]

For context, in #13597, we discuss breaking down the ListItem component into smaller components. It frees us from this problem.

<nav>
  <List>
    <ListItem>
      <ListItemButton component={NavLink}>
        Foo
      </ListItemButton>
      <ListItemSecondaryAction>
        Bar
      </ListItemSecondaryAction>
    </ListItem>
  </List>
</nav>

[oliviertassinari]

@ricosandyca Would it solve #14971? If it does, trading soundness could make sense for an issue with 35 upvotes. Maybe we should add a test case that illustrates the fix?

[ricosandyca]

@oliviertassinari i think it would, and for the test case i think it would be great

[oliviertassinari]

💯 for accepting the changes if:

• It fixes Typescript: in ListItem, property button is no more boolean #14971
• We add a test case to guarantee it.
• We add a test case to surface the limitation of the approach, where it's less sound.

@eps1lon What do you think?

[eps1lon]

It fixes #14971

It doesn't fix it. It just silences the error which can already be achieved by passing <ListItem button={someBoolean as false} />.

I already know that people don't want to do it because of "type casting is bad" but type casting isn't bad inherently. It's only bad if it leads to unsound types and is effectively equivalent to this change proposed here.

But maybe I'm wrong but we can only check that if we have a type test for using HTMLListElement props on <ListItem button={someBoolean} />.

[oliviertassinari]

Should we close the pull request?