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

Open
#398 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
48/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Quiet
Tech stack
react, typescript
Domain
frontend

Research direction

Start in src/MasterDetailLayout.tsx, reading validateChildren and the nearby areChildrenDifferent function. Run the Vite reproduction against the listed React versions and compare direct, wrapped, memoized, and forwarded children. Done means valid composed slot usage no longer blanks the subtree while genuinely invalid children still receive a clear diagnostic.

Written by the indexing model from the issue text.

Description

bug Impact: Low Severity: Major
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
  • #317 asked for this check ("Throw an error if any other type of child component is used"), to
    make the wrapper-component API discoverable. Nothing proposed below removes that diagnostic — it only
    stops it firing on children that are the wrapper components.

  • #315 / #313 are the same identity assumption in the sibling function areChildrenDifferent,
    ~30 lines up, failing in the opposite direction: a router wraps every child view in a provider, so
    the type never changes and the view transition never starts. Two functions, one premise — that
    child.type identity carries semantic meaning about what the child is — and both break as soon as
    anything wraps children.

    assumption effect of wrapping
    validateChildren identity ⇒ "this is a slot component" type differs → throws on valid code
    areChildrenDifferent (#315) identity change ⇒ "the detail changed" type never changes → no transition, silently

    The two do not collide today, which is why this is filed separately: #315's pattern puts the router
    outlet inside .Detail, and validateChildren only inspects the direct children of
    MasterDetailLayout.

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 #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.
Dominant language
TypeScript
Stars
18
Forks
4
Avg merge
52m
Merged PRs (30d)
7

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from vaadin/react-components

All issues in vaadin/react-components

Similar issues

More TypeScript issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.