[TreeItem] No conditional rendering support

[gforrest-bw]

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

TreeView and TreeItem throw an error when attempting to conditionally render a TreeItem.

Expected Behavior 🤔

Any TreeItem which is excluded from rendering by a condition (i.e. replaced with null) should not be rendered, remove itself from the tree structure, and no error is thrown.

Steps to Reproduce 🕹

https://codesandbox.io/s/zen-breeze-fh11u

Steps:

1. Render a <TreeView> with child <TreeItem> components
2. Wrap a <TreeItem> in a conditional rendering expression ({!!condition && <TreeItem />})
3. Toggle off the condition. The parent of the TreeItem throws an error (it could be another TreeItem or the top TreeView): Cannot read property 'props' of null

Context 🔦

I'm using TreeView to render a navigation structure in a mobile nav. Depending on the roles and permissions of the user, certain pages are not available in the navigation structure. I therefore have to conditionally render certain tree items based on user information.

From the code it looks like the tree components are assuming that React.Children.map iterables will all be defined, but for conditionally rendered elements the values in the Children list will be null.

Workaround

A workaround is present, but because of the way TreeView works at the moment it's slightly ugly.

You can define a simple wrapper component to handle conditional rendering:

function ConditionalWrapper({
  show,
  children
}: {
  show: boolean;
  children: ReactElement;
  // required due to TreeView idiosyncrasies
  nodeId: string;
}) {
  if (!show) return null;
  return children;
}

You can then use it instead of traditional conditional rendering to avoid the error:

<ConditionalWrapper show={showTreeItem} nodeId="foo">
  <TreeItem nodeId="foo" label="Foo" />
</ConditionalWrapper>

However, as shown above, nodeId is required for both the wrapper and the TreeItem, because TreeView/TreeItem iterates using React.Children naively, assuming all children will be TreeItem components with nodeId props.

That problem is not necessarily related to this issue, though. If there's interest I would open another issue with thoughts about the use of React.Children for this kind of thing (in short, I think children registering via Context is always a more flexible and stable option than reading data from children).

Your Environment 🌎

Tech	Version
Material-UI	v4.9.0
Material-UI/labs	4.0.0-alpha.40
React	v16.9.19
Browser	Chrome 79.0.3945.88
TypeScript	3.8.0

[joshwooding]

This is an easy fix but due to the massive changes in #18357 I want to wait for that to be merged.

[joshwooding]

However, as shown above, nodeId is required for both the wrapper and the TreeItem, because TreeView/TreeItem iterates using React.Children naively, assuming all children will be TreeItem components with nodeId props.

That problem is not necessarily related to this issue, though. If there's interest I would open another issue with thoughts about the use of React.Children for this kind of thing (in short, I think children registering via Context is always a more flexible and stable option than reading data from children).

To touch on this, children already register with context but to populate their children they use React.Children. You could potentially use context fully but you would have to create a provider per item with children and I'm unsure about the performance issues that might cause.

[oliviertassinari]

@joshwooding Tricky problem, at least with the current error, users know upfront it's not supported.

[joshwooding]

@oliviertassinari For now adding defensive logic should provide an easy fix. I’m going to do some investigation on using multiple contexts instead but it doesn’t feel like an amazing avenue to me.

[oliviertassinari]

I'm not sure to follow. If the introspection of the children is required, would a defensive logic hide the problem, without solving it? By hide, I meant, it will be harder for user to figure out what's wrong with their code. By note solving, I meant, the component won't behave as expected (even without the throw).

[gforrest-bw]

If I read it correctly, defensive logic would just mean checking to see if the child is truthy before accessing properties of it.

I'd also be interested in a performance evaluation of per-child Providers. I wouldn't naively anticipate it being much greater impact than maybe causing an additional render per item, which while not amazing, shouldn't be so bad for such minimal DOM structures which most likely won't actually change (since the context is only used for internal book-keeping, so React will just no-op the DOM changes). But that's only a guess.

[joshwooding]

Thinking about this more, context would only solve the problem of a node knowing it's parent not necessarily knowing it's position amongst it's siblings...

[gforrest-bw]

Isn't it the parent's job to know the order of siblings? A node should be able to say "go to the next tree item" without knowing which tree item that is, and let the parent determine where to move focus.

const { isActive, goToNext, goToPrevious, register } = useTreeItem(nodeId);

const [element, setElement] = useState(null);
const elementRef = el => setElement(el);

useEffect(() => {
  register(nodeId, element);
}, [element]);

const onKeyDown = ev => {
  if (ev.key === 'ArrowDown') {
    goToNext();
  } else if (ev.key === 'ArrowUp') {
    goToPrevious();
  }
}

return <Component ref={elementRef} onKeyDown={onKeyDown} isActive={isActive} {/* etc */} />;

I admit I'm not very familiar with the existing implementation details.

[joshwooding]

That's pretty much how to current implementation works. The problem is that the parent only knows the order of the children by looking at the dom structure, (which might be nested) or the component order. I haven't figured out a great way to not depend on React.Children when doing this.

[gforrest-bw]

I see. That is a tough one.

Examining the nested child structure sorta seems like the most stable approach to me. Recomputing the child order would only have to happen when a child registers/unregisters, not every render.

In fact, now that I'm thinking about it, I've actually done this before. The project was closed-source, so I'll just post the relevant stuff in a gist...

https://gist.github.com/gforrest-bw/d84a04dbcd4f07cf25db77f200fe5295

This implementation was for an Autocomplete/Suggestions system (before MUI added theirs to Labs), but the concept should be generalizable.

The basic idea is to attach a data- attribute to each child item, no matter where it is in the DOM tree. Then, the child registers to its parent context. Optionally, the user can hard-specify the index ordering for full control (like for virtualized UI, where DOM ordering may not be reliable). If they don't, the parent context will automatically walk the DOM whenever the registrations are modified and rebuild the child ordering.

It may not be totally refined, but it did work. This whole system may be useful enough that I should extract it into a library... I may do that... Obviously there are scaling issues as the child DOM gets more complex, but realistically these systems tend to happen near the bottom of the tree. People don't put keyboard-interactive focus managers at the root of their App... hopefully...

Anyways, I think the practical solution today would just be to defend against null, but I think there is potential for improvement using some advanced techniques, provided they are reliable enough.