Renderers fully replace DOM, losing state such as focus [1-2 days]

[sissbruecker]

Currently all renderers (grid cells, dashboard widgets, ...) are treated as actual components rather than just functions that render children. That means that the React virtual DOM will contain an instance of that component, rather than just the children that the function returns. That is problematic with how we advertise using renderers in the docs:

<Grid items={items.value}>
  <GridColumn header="Employee">
    {({ item }) => <span class="foo">{item.fullName}</span>}
  </GridColumn>

  ...
</Grid>

• With this example, it creates a new function on each render cycle
• Since we treat those as a component, it means it actually creates a new component type each render cycle
• So the virtual DOM contains a different type of component on each render cycle
• That results in React fully replacing the actual DOM, and any state is lost (focus, text selection, entered text)

From just looking at the code, this is also easy to miss:

• It's not obvious that the function is now an actual component and that it changes how React updates the DOM
• A render function is a common concept in the React ecosystem and the example above looks like one. However a render function is typically just called directly instead of creating a component instance from it

Option 1: Treat renderers as just render functions

We should consider treating renderers as as just render functions in their traditional sense. So if the renderer is a function, we just call that function instead of creating a component instance with React.createElement.

By doing that, the virtual DOM will always contain the same type of component. For example, if a function returns a button, then the virtual DOM will always just contain a button. That allows React to update the current DOM instead of replacing it completely.

We need to check if this can be done without breaking changes:

• We currently allow passing any type of React component, included class-based components, which can not be called as a function. As a workaround, we might be able to render as component as we do now, but then discard it and instead add its children to the virtual DOM.
• Long term, we should only allow passing functions.
• Need to check if there is some way to use refs with renderers that could break.

Option 2: Improve docs to clarify that renderers are components

If we actually want renderers to be components, we could update our docs make sure that users are guided to never recreate renderers on each render cycle. For example by memoizing the component like so:

const employeeRenderer = useCallback(
  ({ item }) => <span class="foo">{item.fullName}</span>,
  []
);
    
<Grid items={items.value}>
  <GridColumn header="Employee">
    {employeeRenderer}
  </GridColumn>

  ...
</Grid>

However, using components make trivial use-cases more complex. For example:

const [selectedItems, setSelectedItems] = useState<Person[]>();
	
const selectionRenderer = useCallback(
  ({ item }) => (selectedItems.includes(item) ? <SelectedIcon> : <UnselectedIcon>),
  []
);
    
<Grid items={items.value}>
  <GridColumn>
    {selectionRenderer}
  </GridColumn>

  ...
</Grid>

Here a renderer uses the selection state from the component that it's used in. The problem is that it doesn't actually work, because the renderer component will always reference the initial selection state. To fix this, a new renderer component would have to be created each time the selection changes, which brings us back to the initial problem. There are ways to solve this using React contexts or signals, but still:

• The complexity increases
• There is potential for subtle bugs

[yuriy-fix]

Let's invest 1-2 days into researching the option 1 and if it would cause breaking changes.

[vursen]

I'm not sure why class-based components are a concern for option 1. Classes are typically declared outside of the React render scope, so their reference should remain stable across renders, shouldn't it? 🤔

[sissbruecker]

The initial idea with option 1 was to change the type for renderers so that it only accepts a function. Since that would be a breaking change we'll probably need a workaround for now, which could be:

• Call the function if it is a function, otherwise use React.createElement if it's a class
• Use React.createElement always, and then extract children from the rendered component instance

Both are maybe confusing in that the type says it wants a React component, but then we just treat it as something that produces children. I can't think of any practical concerns of not adding an instance of the component to the VDOM, but that would require some testing to figure out if there are side-effects of that.

[vursen]

Call the function if it is a function, otherwise use React.createElement if it's a class

Based on my research, calling renderer functions directly appears to be unsafe, as they might use hooks, which are only allowed inside components. There is a warning that React displays if a function uses hooks and is called directly:

Some components like AutoGrid already use hooks in their renderer functions, so this will be a breaking change. To avoid it, we need a way to distinguish between callbacks that return a ReactNode and functions that are React components. Since FunctionComponent<P> is defined as (props: P) => ReactNode, this distinction doesn't seem to be possible unless we introduce another function for wrapping callbacks that will mark them as such at runtime:

export type ReactRenderer<P> = ReactRendererCallback<P> | ComponentClass<P>;

export interface ReactRendererCallback<P> {
  (props: P): ReactNode;
  [REACT_RENDERER_CALLBACK]?: unknown; // <---- This property can be made required in the next major
}

export function createRendererCallback<P>(callback: (props: P) => ReactNode): ReactRendererCallback<P> {
  const rendererCallback = (props: P) => callback(props);
  rendererCallback[REACT_RENDERER_CALLBACK] = true;
  return rendererCallback;
}

<Grid items={items}>
  <GridColumn>
    {createRendererCallback(({ item }) => <>...</>)}
  </GridColumn>
</Grid>

This explicit approach offers several advantages:

• Ensures backward compatibility
• Doesn't require dependency arrays
• Gives us the option to make this wrapper mandatory in the future e.g. in the next major

The drawbacks are that it makes the code more verbose and is probably not a widely used approach in React.

For more details, check out this branch with a prototype: https://github.com/vaadin/react-components/compare/fix/do-not-re-render-renderers-on-parent-render.

Use React.createElement always, and then extract children from the rendered component instance

My research didn't include this option since I wasn't sure I completely understand it. Was the idea to extract children from the DOM or the Virtual DOM?

[sissbruecker]

Good point, treating components as functions indeed doesn't work with hooks. Regarding the extracting children approach, I was assuming that it would be possible to extract the children from a React component rendered with createElement, however it appears that is not possible.

The approach outlined above seems like a reasonable compromise though having to wrap a render function is indeed odd and not very discoverable. An alternative could be to add a new API where render functions must always be passed as a separate prop (not through children), and which use a different naming convention than the current renderer props (e.g. renderSomething instead of somethingRenderer). Then deprecate the current API, log warnings around its usage and update docs examples. Once the old API is eventually removed we could restore passing a render function as child. But that would result in a lot of effort for users and ourselves.

[vursen]

The approach outlined above seems like a reasonable compromise though having to wrap a render function is indeed odd and not very discoverable

To slightly improve the discoverability, we could display a warning that component functions are deprecated when passed as a renderer. In the next major, we could throw an exception to fully disallow their usage, but with a hint in the exception message.

An alternative could be to add a new API where render functions must always be passed as a separate prop

That's also an option, I think 👍