Development warning for conditionally-rendered panels (#174)

Development warning added to PanelGroup for conditionally-rendered Panel(s) that don't have 'id' and 'order' props
bvaughn · Jul 15, 2023 · acc5362 · acc5362 · vercel · Jul 15, 2023
2 parents 759ad56 + f31d226
commit acc5362
Show file tree

Hide file tree

Showing 12 changed files with 215 additions and 23 deletions.
diff --git a/package.json b/package.json
@@ -22,6 +22,7 @@
   },
   "devDependencies": {
     "@babel/preset-typescript": "^7.21.5",
+    "@playwright/test": "^1.35.0",
     "@types/node": "^18.15.11",
     "@types/react": "^18.0.26",
     "@types/react-dom": "^18.0.10",

diff --git a/packages/react-resizable-panels-website/index.html b/packages/react-resizable-panels-website/index.html
@@ -1,4 +1,4 @@
-<!DOCTYPE html>
+<!doctype html>
 <html>
   <html>
     <head>

diff --git a/packages/react-resizable-panels-website/package.json b/packages/react-resizable-panels-website/package.json
@@ -17,7 +17,6 @@
     "@codemirror/language": "latest",
     "@codemirror/state": "latest",
     "@lezer/highlight": "latest",
-    "@playwright/test": "^1.35.0",
     "kill-port": "latest",
     "localforage": "latest",
     "match-sorter": "latest",

diff --git a/packages/react-resizable-panels-website/playwright.config.ts b/packages/react-resizable-panels-website/playwright.config.ts
@@ -4,6 +4,7 @@ const { DEBUG } = process.env;
 
 const config: PlaywrightTestConfig = {
   use: {
+    browserName: "chromium",
     headless: true,
     viewport: { width: 400, height: 300 },
     ignoreHTTPSErrors: true,
@@ -19,7 +20,6 @@ const config: PlaywrightTestConfig = {
 if (process.env.DEBUG) {
   config.use = {
     ...config.use,
-    browserName: "chromium",
     headless: false,
 
     launchOptions: {

diff --git a/packages/react-resizable-panels-website/src/routes/examples/Conditional.tsx b/packages/react-resizable-panels-website/src/routes/examples/Conditional.tsx
@@ -66,19 +66,19 @@ function Content({
       >
         {showLeftPanel && (
           <>
-            <Panel className={styles.Panel} order={1}>
+            <Panel className={styles.Panel} id="left" order={1}>
               <div className={styles.Centered}>left</div>
             </Panel>
             <ResizeHandle className={styles.ResizeHandle} />
           </>
         )}
-        <Panel className={styles.Panel} order={2}>
+        <Panel className={styles.Panel} id="center" order={2}>
           <div className={styles.Centered}>middle</div>
         </Panel>
         {showRightPanel && (
           <>
             <ResizeHandle className={styles.ResizeHandle} />
-            <Panel className={styles.Panel} order={3}>
+            <Panel className={styles.Panel} id="right" order={3}>
               <div className={styles.Centered}>right</div>
             </Panel>
           </>
@@ -92,19 +92,19 @@ const CODE = `
 <PanelGroup autoSaveId="conditional" direction="horizontal">
   {showLeftPanel && (
     <>
-      <Panel order={1}>
+      <Panel id="left" order={1}>
         left
       </Panel>
       <PanelResizeHandle />
     </>
   )}
-  <Panel order={2}>
+  <Panel id="center" order={2}>
     middle
   </Panel>
   {showRightPanel && (
     <>
       <PanelResizeHandle />
-      <Panel order={3}>
+      <Panel id="right" order={3}>
         right
       </Panel>
     </>

diff --git a/packages/react-resizable-panels-website/src/routes/examples/ExternalPersistence.tsx b/packages/react-resizable-panels-website/src/routes/examples/ExternalPersistence.tsx
@@ -45,10 +45,11 @@ function Content() {
     () => ({
       getItem(name: string) {
         try {
-          const parsed = JSON.parse(
-            decodeURI(window.location.hash.substring(1))
-          );
-          return parsed[name] || "";
+          const raw = decodeURI(window.location.hash.substring(1));
+          if (raw) {
+            const parsed = JSON.parse(raw);
+            return parsed[name] || "";
+          }
         } catch (error) {
           console.error(error);
 

diff --git a/packages/react-resizable-panels-website/tests/DevelopmentWarnings.spec.ts b/packages/react-resizable-panels-website/tests/DevelopmentWarnings.spec.ts
@@ -0,0 +1,144 @@
+import { expect, test } from "@playwright/test";
+import { createElement } from "react";
+import { Panel, PanelGroup, PanelResizeHandle } from "react-resizable-panels";
+
+import { goToUrl, updateUrl } from "./utils/url";
+
+function createElements({
+  numPanels,
+  omitIdProp = false,
+  omitOrderProp = false,
+}: {
+  numPanels: 1 | 2;
+  omitIdProp?: boolean;
+  omitOrderProp?: boolean;
+}) {
+  const panels = [
+    createElement(Panel, {
+      collapsible: true,
+      defaultSize: numPanels === 2 ? 50 : 100,
+      id: omitIdProp ? undefined : "left",
+      order: omitOrderProp ? undefined : 1,
+    }),
+  ];
+
+  if (numPanels === 2) {
+    panels.push(
+      createElement(PanelResizeHandle, { id: "right-handle" }),
+      createElement(Panel, {
+        collapsible: true,
+        defaultSize: 50,
+        id: omitIdProp ? undefined : "right",
+        order: omitOrderProp ? undefined : 2,
+      })
+    );
+  }
+
+  return createElement(
+    PanelGroup,
+    { direction: "horizontal", id: "group" },
+    ...panels
+  );
+}
+
+test.describe("Development warnings", () => {
+  test.describe("conditional panels", () => {
+    test("should warning about missing id props", async ({ page }) => {
+      await goToUrl(
+        page,
+        createElements({
+          omitIdProp: true,
+          numPanels: 1,
+        })
+      );
+
+      const warnings: string[] = [];
+      page.on("console", (message) => {
+        if (message.type() === "warning") {
+          warnings.push(message.text());
+        }
+      });
+
+      await updateUrl(
+        page,
+        createElements({
+          omitIdProp: true,
+          numPanels: 2,
+        })
+      );
+      expect(warnings).toHaveLength(1);
+      expect(warnings[0]).toContain("id and order props recommended");
+
+      await updateUrl(
+        page,
+        createElements({
+          omitIdProp: true,
+          numPanels: 1,
+        })
+      );
+      expect(warnings).toHaveLength(1);
+    });
+
+    test("should warning about missing order props", async ({ page }) => {
+      await goToUrl(
+        page,
+        createElements({
+          omitOrderProp: true,
+          numPanels: 1,
+        })
+      );
+
+      const warnings: string[] = [];
+      page.on("console", (message) => {
+        if (message.type() === "warning") {
+          warnings.push(message.text());
+        }
+      });
+
+      await updateUrl(
+        page,
+        createElements({
+          omitOrderProp: true,
+          numPanels: 2,
+        })
+      );
+      expect(warnings).toHaveLength(1);
+      expect(warnings[0]).toContain("id and order props recommended");
+
+      await updateUrl(
+        page,
+        createElements({
+          omitOrderProp: true,
+          numPanels: 1,
+        })
+      );
+      expect(warnings).toHaveLength(1);
+    });
+
+    test("should not warn if id an order props are specified", async ({
+      page,
+    }) => {
+      await goToUrl(
+        page,
+        createElements({
+          numPanels: 1,
+        })
+      );
+
+      const warnings: string[] = [];
+      page.on("console", (message) => {
+        if (message.type() === "warning") {
+          warnings.push(message.text());
+        }
+      });
+
+      await updateUrl(
+        page,
+        createElements({
+          numPanels: 2,
+        })
+      );
+      expect(warnings).toHaveLength(0);
+    });
+  });
+});
diff --git a/packages/react-resizable-panels/CHANGELOG.md b/packages/react-resizable-panels/CHANGELOG.md
@@ -1,6 +1,7 @@
 # Changelog
 
 ## (Pending)
+* [172](https://github.com/bvaughn/react-resizable-panels/issues/172): Development warning added to `PanelGroup` for conditionally-rendered `Panel`(s) that don't have `id` and `order` props
 * [156](https://github.com/bvaughn/react-resizable-panels/pull/156): Package exports now used to select between node (server-rendering) and browser (client-rendering) bundles
 
 ## 0.0.53

diff --git a/packages/react-resizable-panels/src/Panel.ts b/packages/react-resizable-panels/src/Panel.ts
@@ -125,6 +125,7 @@ function PanelWithForwardedRef({
     collapsible: boolean;
     defaultSize: number | null;
     id: string;
+    idWasAutoGenerated: boolean;
     maxSize: number;
     minSize: number;
     order: number | null;
@@ -134,6 +135,7 @@ function PanelWithForwardedRef({
     collapsible,
     defaultSize,
     id: panelId,
+    idWasAutoGenerated: idFromProps == null,
     maxSize,
     minSize,
     order,
@@ -146,6 +148,7 @@ function PanelWithForwardedRef({
     panelDataRef.current.collapsible = collapsible;
     panelDataRef.current.defaultSize = defaultSize;
     panelDataRef.current.id = panelId;
+    panelDataRef.current.idWasAutoGenerated = idFromProps == null;
     panelDataRef.current.maxSize = maxSize;
     panelDataRef.current.minSize = minSize;
     panelDataRef.current.order = order;

diff --git a/packages/react-resizable-panels/src/PanelGroup.ts b/packages/react-resizable-panels/src/PanelGroup.ts
@@ -161,6 +161,16 @@ function PanelGroupWithForwardedRef({
   // This has the benefit of causing force-collapsed panels to spring back open if drag is reversed.
   const initialDragStateRef = useRef<InitialDragState | null>(null);
 
+  const devWarningsRef = useRef<{
+    didLogDefaultSizeWarning: boolean;
+    didLogIdAndOrderWarning: boolean;
+    prevPanelIds: string[];
+  }>({
+    didLogDefaultSizeWarning: false,
+    didLogIdAndOrderWarning: false,
+    prevPanelIds: [],
+  });
+
   // Use a ref to guard against users passing inline props
   const callbacksRef = useRef<{
     onLayout: PanelGroupOnLayout | undefined;
@@ -334,6 +344,34 @@ function PanelGroupWithForwardedRef({
       }
       debounceMap[autoSaveId](autoSaveId, panelsArray, sizes, storage);
     }
+
+    if (isDevelopment) {
+      const { didLogIdAndOrderWarning, prevPanelIds } = devWarningsRef.current;
+      if (!didLogIdAndOrderWarning) {
+        const { panels } = committedValuesRef.current;
+
+        const panelIds = Array.from(panels.keys());
+
+        devWarningsRef.current.prevPanelIds = panelIds;
+
+        const panelsHaveChanged =
+          prevPanelIds.length > 0 && !areEqual(prevPanelIds, panelIds);
+        if (panelsHaveChanged) {
+          if (
+            Array.from(panels.values()).find(
+              (panel) =>
+                panel.current.idWasAutoGenerated || panel.current.order == null
+            )
+          ) {
+            devWarningsRef.current.didLogIdAndOrderWarning = true;
+
+            console.warn(
+              `WARNING: Panel id and order props recommended when panels are dynamically rendered`
+            );
+          }
+        }
+      }
+    }
   }, [autoSaveId, panels, sizes, storage]);
 
   const getPanelStyle = useCallback(
@@ -344,10 +382,15 @@ function PanelGroupWithForwardedRef({
       // This includes server rendering.
       // At this point the best we can do is render everything with the same size.
       if (panels.size === 0) {
-        if (isDevelopment && !isBrowser && defaultSize == null) {
-          console.warn(
-            `WARNING: Panel defaultSize prop recommended to avoid layout shift after server rendering`
-          );
+        if (isDevelopment) {
+          if (!devWarningsRef.current.didLogDefaultSizeWarning) {
+            if (!isBrowser && defaultSize == null) {
+              devWarningsRef.current.didLogDefaultSizeWarning = true;
+              console.warn(
+                `WARNING: Panel defaultSize prop recommended to avoid layout shift after server rendering`
+              );
+            }
+          }
         }
 
         return {

diff --git a/packages/react-resizable-panels/src/types.ts b/packages/react-resizable-panels/src/types.ts
@@ -24,6 +24,7 @@ export type PanelData = {
     collapsible: boolean;
     defaultSize: number | null;
     id: string;
+    idWasAutoGenerated: boolean;
     maxSize: number;
     minSize: number;
     order: number | null;