fix(codemods): wrap bare function references passed to onAction

packages/codemods/src/migrations.generated.ts

@@ -227,7 +227,7 @@ export const migrations: Omit<MigrationEntry, "body">[] = [
     action: "codemod",
     remotePackage: true,
     apply:
-      "Rename the `action` prop on `Action` to `onAction`. Not only a rename: the new prop is typed `ActionFn` (`(...args: unknown[]) => unknown`), so a function *reference* that declares a parameter no longer type-checks and needs wrapping — `onAction={() => controller.close()}` rather than `onAction={controller.close}`. Check every site where you passed a reference rather than an inline arrow; the codemod renames the prop but cannot decide this one from the source.",
+      "Rename the `action` prop on `Action` to `onAction`. Not only a rename: the new prop is typed `ActionFn` (`(...args: unknown[]) => unknown`), so a function *reference* that declares a parameter no longer type-checks and needs wrapping — `onAction={() => controller.close()}` rather than `onAction={controller.close}`. A codemod does both. It wraps every bare reference (`close`, `controller.close`), because the wrap is a no-op for a reference that did not need it. It leaves a value that already is the handler or produces one — an arrow function, a function expression, a call like `makeHandler()` or `close.bind(controller)` — and anything that is not one reference, such as `isOpen ? close : open` or `controller?.close`. Two wraps to look at afterwards: a handler that read the event `Action` forwards stops receiving it, and a possibly-undefined reference (`onAction={props.onAction}`) becomes a call TypeScript rejects — add the guard it asks for.",
   },
   {
     id: "button-props-interfaces",

packages/codemods/src/migrations/action-prop-to-on-action/entry.md

@@ -9,9 +9,15 @@ apply: >-
   prop is typed `ActionFn` (`(...args: unknown[]) => unknown`), so a function
   *reference* that declares a parameter no longer type-checks and needs wrapping
   — `onAction={() => controller.close()}` rather than
-  `onAction={controller.close}`. Check every site where you passed a reference
-  rather than an inline arrow; the codemod renames the prop but cannot decide
-  this one from the source.
+  `onAction={controller.close}`. A codemod does both. It wraps every bare
+  reference (`close`, `controller.close`), because the wrap is a no-op for a
+  reference that did not need it. It leaves a value that already is the handler
+  or produces one — an arrow function, a function expression, a call like
+  `makeHandler()` or `close.bind(controller)` — and anything that is not one
+  reference, such as `isOpen ? close : open` or `controller?.close`. Two wraps
+  to look at afterwards: a handler that read the event `Action` forwards stops
+  receiving it, and a possibly-undefined reference (`onAction={props.onAction}`)
+  becomes a call TypeScript rejects — add the guard it asks for.
 ---
 
 `Action`'s `action` prop is now called `onAction`, which matches the naming of
@@ -52,7 +58,22 @@ Only function **references** are affected. An inline arrow
 (`onAction={() => …}`), a zero-parameter function, and anything already
 accepting `unknown` are all fine.
 
-A codemod renames the prop. It deliberately does not wrap: whether the
-referenced function declares a parameter cannot be decided from the source —
-that needs type information — and wrapping everything would silently drop the
-arguments `Action` passes to handlers that do accept them.
+A codemod renames the prop and wraps the reference.
+
+Whether a reference _needs_ wrapping is not decidable from the source — that
+needs type information. Performing the wrap does not need it: `() => fn()` calls
+what `Action` would have called, and `onAction` takes no arguments. So the wrap
+fixes the reference that needed it and changes nothing for the rest, which makes
+the decision unnecessary.
+
+Wrapped: a plain identifier and a member expression (`close`,
+`controller.close`, `this.handleSave`). Left alone: an arrow function and a
+function expression, which already are the handler; a call (`makeHandler()`,
+`close.bind(controller)`), which produces it; and anything that is not one
+reference (`isOpen ? close : open`, `onClose ?? noop`, `controller?.close`).
+
+Two wraps are worth a look afterwards. A handler that read the event `Action`
+forwards — undocumented, but it does forward the trigger's event — stops
+receiving it. And `onAction={props.onAction}`, where the reference may be
+`undefined`: passing it was fine, calling it is not, so TypeScript now reports
+the call and wants a guard.

packages/codemods/src/migrations/action-prop-to-on-action/transform.test.ts

@@ -20,18 +20,193 @@ export const A = () => (
 
 export const A = () => (
   <>
-    <Action onAction={run} />
-    <Action onAction={run} />
+    <Action onAction={() => run()} />
+    <Action onAction={() => run()} />
   </>
 );
 `);
   });
+
+  test("wraps a bare identifier in a call", () => {
+    const source = `import { Action } from "@mittwald/flow-react-components";
+
+export const A = () => <Action action={close} />;
+`;
+
+    expect(runTransform(transform, source)).toContain(
+      `onAction={() => close()}`,
+    );
+  });
+
+  test("wraps a member expression, the case that motivated this", () => {
+    // `controller.close` is typed `(options?: CloseOverlayOptions) => void`,
+    // which is not assignable to `ActionFn`. Wrapping is what fixes it.
+    const source = `import { Action } from "@mittwald/flow-react-components";
+
+export const A = () => <Action action={controller.close} />;
+`;
+
+    expect(runTransform(transform, source)).toContain(
+      `onAction={() => controller.close()}`,
+    );
+  });
+
+  test("wraps a deep member expression and one off `this`", () => {
+    const source = `import { Action } from "@mittwald/flow-react-components";
+
+export const A = () => (
+  <>
+    <Action action={store.modal.controller.close} />
+    <Action action={this.handleSave} />
+  </>
+);
+`;
+
+    const result = runTransform(transform, source);
+
+    expect(result).toContain(`onAction={() => store.modal.controller.close()}`);
+    expect(result).toContain(`onAction={() => this.handleSave()}`);
+  });
+
+  test("wraps a bare reference already written as onAction", () => {
+    // A consumer who renamed the prop by hand — or ran an earlier version of
+    // this codemod — still has the type error. The prop name is not what
+    // decides the wrap; the value is.
+    const source = `import { Action } from "@mittwald/flow-react-components";
+
+export const A = () => <Action onAction={controller.close} />;
+`;
+
+    expect(runTransform(transform, source)).toContain(
+      `onAction={() => controller.close()}`,
+    );
+  });
+
+  test("leaves an inline arrow alone", () => {
+    const source = `import { Action } from "@mittwald/flow-react-components";
+
+export const A = () => (
+  <>
+    <Action action={() => controller.close()} />
+    <Action action={async () => await save()} />
+    <Action action={(...args) => log(args)} />
+  </>
+);
+`;
+
+    expect(runTransform(transform, source)).toBe(
+      source.replace(/action=/g, "onAction="),
+    );
+  });
+
+  test("leaves a function expression alone", () => {
+    const source = `import { Action } from "@mittwald/flow-react-components";
+
+export const A = () => (
+  <>
+    <Action action={function () { close(); }} />
+    <Action action={function named() { close(); }} />
+  </>
+);
+`;
+
+    expect(runTransform(transform, source)).toBe(
+      source.replace(/action=/g, "onAction="),
+    );
+  });
+
+  test("leaves a call alone — it produces the handler, it is not the handler", () => {
+    // `action={makeHandler()}` already evaluates to a function. Wrapping it
+    // would call `makeHandler` on trigger and throw the handler away.
+    const source = `import { Action } from "@mittwald/flow-react-components";
+
+export const A = () => (
+  <>
+    <Action action={makeHandler()} />
+    <Action action={close.bind(controller)} />
+    <Action action={useCallback(close, [])} />
+  </>
+);
+`;
+
+    expect(runTransform(transform, source)).toBe(
+      source.replace(/action=/g, "onAction="),
+    );
+  });
+
+  test("leaves anything that is not a plain reference alone", () => {
+    // A conditional or a fallback chain is not a bare reference, so wrapping it
+    // would be a guess about which branch is the handler.
+    const source = `import { Action } from "@mittwald/flow-react-components";
+
+export const A = () => (
+  <>
+    <Action action={isOpen ? close : open} />
+    <Action action={onClose ?? noop} />
+    <Action action={controller?.close} />
+  </>
+);
+`;
+
+    expect(runTransform(transform, source)).toBe(
+      source.replace(/action=/g, "onAction="),
+    );
+  });
+
+  test("resolves an aliased and a namespace import", () => {
+    const source = `import { Action as FlowAction } from "@mittwald/flow-react-components";
+import * as Flow from "@mittwald/flow-remote-react-components";
+
+export const A = () => (
+  <>
+    <FlowAction action={close} />
+    <Flow.Action action={controller.close} />
+  </>
+);
+`;
+
+    const result = runTransform(transform, source);
+
+    expect(result).toContain(`<FlowAction onAction={() => close()} />`);
+    expect(result).toContain(
+      `<Flow.Action onAction={() => controller.close()} />`,
+    );
+  });
+
+  test("leaves another package's Action and a plain form alone", () => {
+    const source = `import { Action } from "some-other-package";
+
+export const A = () => (
+  <>
+    <Action action={close} />
+    <form action={submitUrl} />
+  </>
+);
+`;
+
+    expect(runTransform(transform, source)).toBe(source);
+  });
+
+  test("leaves the props beside onAction untouched", () => {
+    const source = `import { Action } from "@mittwald/flow-react-components";
+
+export const A = () => <Action action={close} showFeedback break skip={2} />;
+`;
+
+    expect(runTransform(transform, source)).toContain(
+      `<Action onAction={() => close()} showFeedback break skip={2} />`,
+    );
+  });
 });
 
 /**
  * Consumers run codemods one after another, so a second pass over already
  * migrated code has to be a no-op — see `src/tests/transformCoverage.test.ts`
  * for why every transform is required to prove this.
+ *
+ * The wrap is the interesting half here: it has to recognise its own output.
+ * `onAction={() => close()}` is an arrow function, so the second pass leaves it
+ * where it is instead of producing `() => (() => close())()`.
  */
 describe("running it twice changes nothing", () => {
   test("stays idempotent", () => {
@@ -40,8 +215,10 @@ describe("running it twice changes nothing", () => {
 export const A = () => (
   <>
     <Action action={run} />
+    <Action action={controller.close} />
     <Action action={stale} onAction={run} />
-    <Action onAction={run} />
+    <Action onAction={() => run()} />
+    <Action action={() => run()} />
   </>
 );
 `;