vanjs-org / van

🍦 VanJS: World's smallest reactive UI framework. Incredibly Powerful, Insanely Small - Everyone can build a useful UI app in an hour.
https://vanjs.org
MIT License
3.77k stars 87 forks source link

Bad support on conditional rendering when return nulls #337

Closed swwind closed 3 months ago

swwind commented 3 months ago

I wrote the following code

const Hello = () => {
  const flag = van.state(false);

  return [
    button({ onclick: e => flag.val = !flag.val }, "Click me"),
    () => flag.val ? null : p("114514")
  ]
}
  1. Click button: p('114514') disappears as expected
  2. Click button: p('114514') should come back but it doesn't

https://jsfiddle.net/uch6d7es/1/


The code simply suppose the return value of derive is a single Node (including primitive values that can be converted into Text) and perform Node.replaceWith(Node) once the state changes. However if we return a null, we will not get a Node for replacing in next state changes, and thus failed to do anything further.


Simply we can create a Comment Node if get null in conditional rendering to ensure we have a Node to replace in next state change, but I hope we can come up with better solutions for support returning arrays (acts like Fragment) as well.

sirenkovladd commented 3 months ago

For State-derived child nodes, if the binding function returns null or undefined, the DOM node will removed. Removed DOM node will never be brought back, even when the binding function would return a non-null/undefined value based on future values of the dependencies.

https://vanjs.org/tutorial#removing-a-dom-node

sirenkovladd commented 3 months ago

Proposed solutions:

const {button, p, div} = van.tags

const Hello1 = () => {
  const flag = van.state(false);

  return [
    button({ onclick: e => flag.val = !flag.val }, "Click me"),
    () => flag.val ? '' : p("1919810")
  ]
}

const Hello2 = () => {
  const flag = van.state(false);

  return () => div(
    button({ onclick: e => flag.val = !flag.val }, "Click me"),
    flag.val ? null : p("1919810")
  )
}

van.add(document.body, Hello1(), Hello2())

Better Hello1

swwind commented 3 months ago

https://vanjs.org/tutorial#removing-a-dom-node

I see. This feature just sounds weird, seems like the code behaviors like that, and we explain it as a feature! (like BUD in minecraft if you know)

Hello1, Hello2

This seems better to match the design of State-derived child nodes in vanjs. However, I think user will be much happy if we can just support these features but not avoid them.

Anyway, I'll make some changes to adopt my own ideas. This project is amazing, it would be better if not keen on reducing code size.

Tao-VanJS commented 3 months ago

Hi @swwind,

Thanks for your feedback!

Using null or undefined to instruct the permanent removal of a DOM node is an intentional design choice. Alternatively, we could introduce a special remove symbol to instruct the removal. We chose not to introduce an additional symbol since it's not necessary (Occam's razor). Additionally, if an empty string can be used to temporarily remove a DOM node (which can be brought back some time later), it will be redundant for null or undefined to mean the same thing.

Hope it explains well :-)