Skip to content

[MasterDetailLayout] Child validation compares element identity, so the slot components cannot be produced by a wrapper #398

Description

@jcgueriaud1

Description

This is about how the check requested in #317 is implemented, not about whether it should exist. The
goal there — make an unusual, IDE-undiscoverable API shape fail loudly instead of silently — is worth
keeping. But comparing element identity over-rejects: it also refuses code that does exactly what the
error message asks for.

validateChildren in src/MasterDetailLayout.tsx rejects any child whose type is not
identity-equal to Master, Detail or DetailPlaceholder:

function validateChildren(children: React.ReactNode) {
  React.Children.forEach(children, (child) => {
    if (
      React.isValidElement(child) &&
      child.type !== Master &&
      child.type !== Detail &&
      child.type !== DetailPlaceholder
    ) {
      throw new Error('Invalid child in MasterDetailLayout. Only <MasterDetailLayout.Master>, …');
    }
  });
}

Identity equality holds only when the element is created from the very same function object. It does
not hold for a component that renders one of the three internally, nor for one of the three behind
memo or forwardRef — so the throw fires on code whose intent is exactly the documented one.
MasterDetailLayout is the only component in @vaadin/react-components that cannot be composed
indirectly.

Reproduction

npm create vite@latest mdl-repro -- --template react-ts && cd mdl-repro && npm i @vaadin/react-components@25.2.8 @vaadin/aura@25.2.8,
then:

import React, { type PropsWithChildren } from 'react';
import { MasterDetailLayout } from '@vaadin/react-components/MasterDetailLayout.js';

const master = <MasterDetailLayout.Master>master</MasterDetailLayout.Master>;

// 1. documented usage — renders
export const Direct = () => (
  <MasterDetailLayout>
    {master}
    <MasterDetailLayout.Detail>detail</MasterDetailLayout.Detail>
  </MasterDetailLayout>
);

// 2. a wrapper of your own that renders Detail — THROWS
const MyDetail = ({ children }: PropsWithChildren) => <MasterDetailLayout.Detail>{children}</MasterDetailLayout.Detail>;
export const Wrapped = () => (
  <MasterDetailLayout>
    {master}
    <MyDetail>detail</MyDetail>
  </MasterDetailLayout>
);

// 3. React.memo around Detail — THROWS. The child IS Detail, behind a memo object.
const MemoDetail = React.memo(MasterDetailLayout.Detail);
export const Memoized = () => (
  <MasterDetailLayout>
    {master}
    <MemoDetail>detail</MemoDetail>
  </MasterDetailLayout>
);

// 4. a wrapper that FORWARDS children it did not create — renders
const Shell = ({ children }: PropsWithChildren) => <MasterDetailLayout>{children}</MasterDetailLayout>;
export const Forwarded = () => (
  <Shell>
    {master}
    <MasterDetailLayout.Detail>detail</MasterDetailLayout.Detail>
  </Shell>
);

Results on @vaadin/react-components@25.2.8, Chromium, verified under React 19.2.8 and React 18.3.1
(each case mounted behind an error boundary so one throw does not hide the others):

case how the Detail element is produced outcome
1 written at the call site renders
2 rendered inside a wrapper component throws
3 React.memo(MasterDetailLayout.Detail) throws
4 created at the call site, passed through a wrapper renders

Case 4 bounds the problem: forwarding children preserves identity and is fine. What breaks is
producing one of the three slot elements inside another component — the ordinary way to factor out a
layout used on more than one screen.

Case 3 is the clearest evidence that identity is the wrong test: the child is literally Detail, and
the error still says only Detail is allowed.

The same reasoning covers a case that is much harder to debug: if two copies of
@vaadin/react-components end up on the page — a dual ESM/CJS resolution, or a bundle that
externalizes the package while the host also loads it — then MasterDetailLayout and the Master the
caller imported come from different module instances, the identities differ, and correct code throws.

Expected

Producing the three slot components indirectly — a wrapper, memo, forwardRef — should work, as it
does for every other component in the package. A genuinely wrong child should still produce a clear
error.

Why this costs more than it looks

  • It is a throw, not a warning. The subtree unmounts; without an error boundary the app is blank.
  • There is no type-level signal. children is ReactNode, so every case above compiles and then
    fails at runtime. The docs' warning ("Using any other component as a child will throw an error")
    reads as being about mistakes, and gives no hint that a correct wrapper is one.
  • The underlying web component is more permissive than its wrapper. vaadin-master-detail-layout
    takes slot="detail" children, so following the web component's documentation produces React code
    that compiles and throws.
  • It runs on every render, in the component body, in production builds too.

Relationship to existing issues

Caveat worth stating plainly: relaxing this check is necessary but not sufficient for the wrapper
use case. A shared shell that renders Detail internally would pass validation and then hit #315
the same wrapping validation objects to is the wrapping that defeats transition detection. This issue
is the smaller, self-contained half. It is worth fixing on its own because it turns working code into a
blank screen, but it does not resolve #315.

Suggested fix

The aim is to keep #317's diagnostic for genuine mistakes while letting the three slot components be
produced indirectly.

Any check on child.type alone cannot recognise a wrapper — MyDetail is an opaque function, and
what it renders is unknowable without rendering it. So the identity comparison cannot be repaired, only
relaxed. Two changes that together keep the helpful error without blocking composition:

  1. Warn instead of throwing, and only in development. console.error with the same message keeps
    the diagnostic [MasterDetailLayout] Verify proper usage of Master / Detail wrapper components #317 wanted, costs nothing in production, and lets a wrapper work. This is what the
    React ecosystem generally does for "unexpected child" checks.

  2. Recognise the slot components by a marker rather than by identity, so a wrapper can opt in and
    two module instances agree:

    export const MASTER_DETAIL_SLOT = Symbol.for('vaadin.master-detail-layout.slot');
    
    Master[MASTER_DETAIL_SLOT] = 'master';
    Detail[MASTER_DETAIL_SLOT] = 'detail';
    DetailPlaceholder[MASTER_DETAIL_SLOT] = 'detail-placeholder';
    
    // in validateChildren
    const type = child.type as any;
    const slot = type?.[MASTER_DETAIL_SLOT] ?? type?.type?.[MASTER_DETAIL_SLOT]; // unwraps memo/forwardRef
    if (!slot) { /* warn */ }

    Symbol.for is a cross-realm registry, so this survives duplicate copies of the package, and
    unwrapping type.type covers memo and forwardRef with no change at the call site.

Accepting slot="detail" children the way the web component does would also resolve it, and would
close the gap between the two documentation sets.

Environment

  • @vaadin/react-components 25.2.8, @vaadin/master-detail-layout 25.2.8
  • React 19.2.8 and React 18.3.1, Chromium
  • Found while building a component library on top of the React wrappers, where the layout is rendered
    by a shared shell component rather than written out at each call site.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions