test(dummy): address StrictMode review feedback

justin808 · claude · justin808 · commit 3dd51c984539 · 2026-04-28T20:33:17.000-10:00
Picks up the optional-tier review feedback on PR #3206: - RSC bundle now also drops the `react-on-rails-pro/client$` alias so the client-only StrictMode shim cannot resolve inside the React server bundle. - `serverWebpackConfig` writes its alias override via a defensive spread rather than mutating `resolve.alias` directly. - OSS `strictModeSupport` splits the non-function cache into a WeakMap for object components (memo/forwardRef/lazy) and a Map for string keys, so registered objects are no longer pinned for the lifetime of the module. - OSS `isRenderFunction` documents the `length >= 2` heuristic and how to opt out of it. - `client-bundle.js` documents the bundle-scope limitation of the register patch. - Adds a Jest case that pins down 2-arg functional component handling. - React 16 dummy startup files document the intentional cross-tree import of `app/strictModeSupport`. - Pro `wrapRenderFunctionInStrictMode` documents the `length === 2` invariant and why STRICT_MODE_PATCHED keeps it safe. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
diff --git a/react_on_rails/spec/dummy/client/app-react16/startup/ManualRenderApp.jsx b/react_on_rails/spec/dummy/client/app-react16/startup/ManualRenderApp.jsx
@@ -1,5 +1,7 @@
 import React from 'react';
 import ReactDOM from 'react-dom';
+// Intentional cross-tree import: the React 16 dummy entries reuse the StrictMode helper from the
+// React 19 `app/` tree. Keep the import path in sync if `app/strictModeSupport` is moved.
 import { wrapElementInStrictMode } from '../../app/strictModeSupport';
 
 export default (props, _railsContext, domNodeId) => {
diff --git a/react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx b/react_on_rails/spec/dummy/client/app-react16/startup/ReduxApp.client.jsx
@@ -12,6 +12,8 @@ import reducers from '../../app/reducers/reducersIndex';
 import composeInitialState from '../../app/store/composeInitialState';
 
 import HelloWorldContainer from '../../app/components/HelloWorldContainer';
+// Intentional cross-tree import: the React 16 dummy entries reuse the StrictMode helper from the
+// React 19 `app/` tree. Keep the import path in sync if `app/strictModeSupport` is moved.
 import { wrapElementInStrictMode } from '../../app/strictModeSupport';
 
 /*
diff --git a/react_on_rails/spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx b/react_on_rails/spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx
@@ -7,6 +7,8 @@ import ReactOnRails from 'react-on-rails/client';
 import ReactDOM from 'react-dom';
 
 import HelloWorldContainer from '../../app/components/HelloWorldContainer';
+// Intentional cross-tree import: the React 16 dummy entries reuse the StrictMode helper from the
+// React 19 `app/` tree. Keep the import path in sync if `app/strictModeSupport` is moved.
 import { wrapElementInStrictMode } from '../../app/strictModeSupport';
 
 /*
diff --git a/react_on_rails/spec/dummy/client/app/packs/client-bundle.js b/react_on_rails/spec/dummy/client/app/packs/client-bundle.js
@@ -13,10 +13,13 @@ import { wrapRegisteredComponentsWithStrictMode } from '../strictModeSupport';
 
 const STRICT_MODE_PATCHED = '__reactOnRailsDummyStrictModePatched';
 
+// Scope: this patch only affects `ReactOnRails.register` calls that share this bundle's module
+// instance (this pack and inline ERB views that run after it). Separate entry-point packs that
+// import `react-on-rails/client` independently get their own unpatched module and would skip
+// StrictMode wrapping.
 if (!ReactOnRails[STRICT_MODE_PATCHED]) {
   const originalRegister = ReactOnRails.register.bind(ReactOnRails);
 
-  // Covers this bundle and inline ERB register calls, which run after the bundle has loaded.
   ReactOnRails.register = (components) =>
     originalRegister(wrapRegisteredComponentsWithStrictMode(components));
   Object.defineProperty(ReactOnRails, STRICT_MODE_PATCHED, { value: true });
diff --git a/react_on_rails/spec/dummy/client/app/strictModeSupport.jsx b/react_on_rails/spec/dummy/client/app/strictModeSupport.jsx
@@ -1,9 +1,16 @@
 import React from 'react';
 
 const wrappedFunctionComponents = new WeakMap();
-const wrappedOtherComponents = new Map(); // Map, not WeakMap: string component names are valid keys.
+// Object-typed React components (memo, forwardRef, lazy) are GC-safe in a WeakMap.
+const wrappedObjectComponents = new WeakMap();
+// Strings are primitives and cannot key a WeakMap, so registered string component names live here.
+const wrappedStringComponents = new Map();
 
 // Mirrors the public react-on-rails/isRenderFunction convention for this dummy-only wrapper.
+// Note: the `length >= 2` heuristic intentionally classifies any 2-arg function as a render
+// function. Legacy 2-arg components that use the (props, context) signature must opt back in by
+// setting `fn.renderFunction = false` (or, for class components, are detected via the prototype
+// check). Set `fn.renderFunction = true` to flag a 1-arg render function explicitly.
 const isRenderFunction = (component) => {
   if (typeof component !== 'function') {
     return false;
@@ -46,13 +53,24 @@ const wrapComponentInStrictMode = (component) => {
     return wrappedComponent;
   }
 
-  const cachedComponent = wrappedOtherComponents.get(component);
+  if (typeof component === 'string') {
+    const cachedComponent = wrappedStringComponents.get(component);
+    if (cachedComponent) {
+      return cachedComponent;
+    }
+
+    const wrappedComponent = createStrictModeWrapper(component);
+    wrappedStringComponents.set(component, wrappedComponent);
+    return wrappedComponent;
+  }
+
+  const cachedComponent = wrappedObjectComponents.get(component);
   if (cachedComponent) {
     return cachedComponent;
   }
 
   const wrappedComponent = createStrictModeWrapper(component);
-  wrappedOtherComponents.set(component, wrappedComponent);
+  wrappedObjectComponents.set(component, wrappedComponent);
   return wrappedComponent;
 };
 
diff --git a/react_on_rails/spec/dummy/tests/strict-mode-support.test.jsx b/react_on_rails/spec/dummy/tests/strict-mode-support.test.jsx
@@ -64,6 +64,19 @@ describe('strictModeSupport', () => {
     expect(wrappedComponents.rendererFunction).toBe(rendererFunction);
   });
 
+  it('treats a 2-arg functional component as a render function (arity heuristic)', () => {
+    // A 2-arg component looks like a render function to isRenderFunction; this test pins down
+    // the heuristic so a future change has to update the test alongside the behavior.
+    const TwoArgComponent = ({ greeting }, _legacyContext) => <div>{greeting}</div>;
+    TwoArgComponent.propTypes = {
+      greeting: PropTypes.string.isRequired,
+    };
+
+    const wrappedComponents = wrapRegisteredComponentsWithStrictMode({ TwoArgComponent });
+
+    expect(wrappedComponents.TwoArgComponent).toBe(TwoArgComponent);
+  });
+
   it('wraps manual render trees in StrictMode', () => {
     const innerElement = <div>hello</div>;
     const wrappedElement = wrapElementInStrictMode(innerElement);
diff --git a/react_on_rails_pro/spec/dummy/client/app/strictModeSupport.tsx b/react_on_rails_pro/spec/dummy/client/app/strictModeSupport.tsx
@@ -145,6 +145,10 @@ const wrapRenderFunctionResult = (result: unknown): unknown => {
   return result;
 };
 
+// The wrapped function below has `length === 2`, so `isRenderFunction` would re-classify it as a
+// render function if it ever flowed back through `wrapRegisteredComponentsWithStrictMode`. The
+// STRICT_MODE_PATCHED guard on `enableStrictModeForReactOnRails` ensures the patched `register`
+// runs only once per singleton, which keeps that re-entry path unreachable.
 const wrapRenderFunctionInStrictMode = (renderFunction: RenderFunction): RenderFunction => {
   const cachedRenderFunction = wrappedRenderFunctions.get(renderFunction);
   if (cachedRenderFunction) {
diff --git a/react_on_rails_pro/spec/dummy/config/webpack/rscWebpackConfig.js b/react_on_rails_pro/spec/dummy/config/webpack/rscWebpackConfig.js
@@ -36,6 +36,9 @@ const configureRsc = () => {
   const rootNodeModules = resolve(__dirname, '..', '..', '..', '..', '..', 'node_modules');
   const rscAliases = { ...(rscConfig.resolve?.alias || {}) };
   delete rscAliases['react-on-rails-pro$'];
+  // Drop the client-only StrictMode shim so RSC imports of `react-on-rails-pro/client` don't pull
+  // in a browser entry point inside the React server bundle.
+  delete rscAliases['react-on-rails-pro/client$'];
   rscConfig.resolve = {
     ...rscConfig.resolve,
     conditionNames: ['react-server', '...'],
diff --git a/react_on_rails_pro/spec/dummy/config/webpack/serverWebpackConfig.js b/react_on_rails_pro/spec/dummy/config/webpack/serverWebpackConfig.js
@@ -25,14 +25,20 @@ const configureServer = (rscBundle = false) => {
   // entry value will result in changing the client config!
   // Using webpack-merge into an empty object avoids this issue.
   const serverWebpackConfig = commonWebpackConfig();
-  serverWebpackConfig.resolve.alias['react-on-rails-pro$'] = path.resolve(
-    __dirname,
-    '..',
-    '..',
-    'client',
-    'app',
-    'strictModeReactOnRailsProNode.js',
-  );
+  serverWebpackConfig.resolve = {
+    ...serverWebpackConfig.resolve,
+    alias: {
+      ...(serverWebpackConfig.resolve?.alias || {}),
+      'react-on-rails-pro$': path.resolve(
+        __dirname,
+        '..',
+        '..',
+        'client',
+        'app',
+        'strictModeReactOnRailsProNode.js',
+      ),
+    },
+  };
 
   // We just want the single server bundle entry
   const serverEntry = {
