render() returning null or false causes update() to fail

[Gargaj]

Using the following component will cause an exception:

class Empty extends nanoJSX.Component 
{
  didMount()
  {
    this.update();
  }
  render() 
  {
    return null; // or false
  }
}

This is due to https://github.com/nanojsx/nano/blob/master/src/component.ts#L117 assuming there IS at least one element rendered, even though (as far as I understand with general JSX practice) returning null or false in render() to not render any elements is considered valid behaviour (though there seems to be a difference in the behaviour of the two)

[yandeu]

nano-jsx works quite differently from React.

​Based on the nano-jsx documentation:

On every component you call update(), the root element needs to be a DOM element or a Fragment of DOM elements.

​Try to ensure the component returns a single, valid element like a <div> or a <template> (or a Fragment of elements) instead of a boolean or null type.

[yandeu]

We should modify line 117 of src/component.ts to ensure the intended warning message is executed instead of throwing an exception.

[yandeu]

Maybe something like this just above line 117:

if (!oldElements || oldElements.length === 0) {
    return console.warn('Component needs a parent element to get updated! (No previous elements found to update from)')
}

[Gargaj]

If that is the case, maybe the return value of render() should be checked if it's null/false.

[yandeu]

There is a check in core.ts
https://github.com/nanojsx/nano/blob/b6357d8ada74b687d0f527f2c46a31484c4f358b/src/core.ts#L116

[yandeu]

I believe the most appropriate action here is to throw a well-defined exception. This forces the developer to handle the issue and to learn how component updating works in nano-jsx.

// get valid parent node
const parent = oldElements[0]?.parentNode

// make sure we have a parent
if (!parent) {
  const msg = 'Component needs a parent element to get updated! (No previous elements found to update from)'
  console.warn(msg)
  throw new Error(msg)
}

[Gargaj]

There is a check in core.ts

What I meant is to check the return value of render() immediately and warn / throw if it doesn't contain an element, and thus narrow the issue down to that one call; the above exception is good as well in case there's some external Javascript that removes an element, but it doesn't point the finger directly at a faulty render().

[yandeu]

I think a component should be able to return null if it is given the responsibility to decide whether it wants to be rendered or not.

---

In the example below, the component has the responsibility to decide whether to be rendered or not. If the props that <MyName> receives are invalid, the component will simply not be rendered.

class MyName extends Component<{ name: string | undefined | null }> {
  clr = this.randomColor()

  randomColor() {
    const colors = ['red', 'green', 'blue', 'yellow', 'purple', 'orange']
    const color = colors[Math.floor(Math.random() * colors.length)]
    return color
  }

  changeColor() {
    this.clr = this.randomColor()
    this.update()
  }

  render() {
    if (!this.props.name) return null

    return (
      <p onClick={() => this.changeColor()} style={{ color: this.clr }}>
        {this.props.name}
      </p>
    )
  }
}

class App extends Component {
  render() {
    return (
      <div>
        <MyName name="Alice" />
        <MyName name="Bob" />
        <MyName name={null} />
      </div>
    )
  }
}

render(<App />, document.getElementById('root'))

[Gargaj]

I understand that reasoning, but that feels like "under certain circumstances it will work, but mostly it won't"; in our case the scenario is an element that doesn't want to be rendered until certain async information arrives, but knowing the above has to return an empty element instead of null, which feels inconsistent.

Ultimately I'm not too fussed about whether it changes and the exception/warning above is a genuinely welcome clue, just feels like it could be documented more explicitly that returning null in render() is more of a risk than in general React where it's commonplace.

[yandeu]

I did not know that it is common to use null in React.

[Gargaj]

It's in the official docs: https://react.dev/learn/conditional-rendering#conditionally-returning-nothing-with-null (The docs say it's "not common" but I think that's more wishful thinking on their end 🤭 )

[yandeu]

After a lot of thought, I believe you are right. A future release of nano-jsx needs to be stricter and clearer about what is allowed. Currently, the library tries to render everything, but this much flexibility is costly, and good TypeScript types are especially suffering from this high degree of freedom.