diff --git a/packages/codemods/src/migrations.generated.ts b/packages/codemods/src/migrations.generated.ts index a583bf303a..ec8ea9fc88 100644 --- a/packages/codemods/src/migrations.generated.ts +++ b/packages/codemods/src/migrations.generated.ts @@ -237,7 +237,7 @@ export const migrations: Omit[] = [ 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", diff --git a/packages/codemods/src/migrations/action-prop-to-on-action/entry.md b/packages/codemods/src/migrations/action-prop-to-on-action/entry.md index 27ee1d1994..9d21ee1eef 100644 --- a/packages/codemods/src/migrations/action-prop-to-on-action/entry.md +++ b/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. diff --git a/packages/codemods/src/migrations/action-prop-to-on-action/transform.test.ts b/packages/codemods/src/migrations/action-prop-to-on-action/transform.test.ts index 14f8bff5b3..41059edb65 100644 --- a/packages/codemods/src/migrations/action-prop-to-on-action/transform.test.ts +++ b/packages/codemods/src/migrations/action-prop-to-on-action/transform.test.ts @@ -20,18 +20,193 @@ export const A = () => ( export const A = () => ( <> - - + run()} /> + run()} /> ); `); }); + + test("wraps a bare identifier in a call", () => { + const source = `import { Action } from "@mittwald/flow-react-components"; + +export const A = () => ; +`; + + 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 = () => ; +`; + + 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 = () => ( + <> + + + +); +`; + + 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 = () => ; +`; + + 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 = () => ( + <> + controller.close()} /> + await save()} /> + 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 = () => ( + <> + + + +); +`; + + 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 = () => ( + <> + + + + +); +`; + + 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 = () => ( + <> + + + + +); +`; + + 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 = () => ( + <> + + + +); +`; + + const result = runTransform(transform, source); + + expect(result).toContain(` close()} />`); + expect(result).toContain( + ` controller.close()} />`, + ); + }); + + test("leaves another package's Action and a plain form alone", () => { + const source = `import { Action } from "some-other-package"; + +export const A = () => ( + <> + +
+ +); +`; + + 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 = () => ; +`; + + expect(runTransform(transform, source)).toContain( + ` 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 = () => ( <> + - + run()} /> + run()} /> ); `; diff --git a/packages/codemods/src/migrations/action-prop-to-on-action/transform.ts b/packages/codemods/src/migrations/action-prop-to-on-action/transform.ts index 932be29a09..764756a1cb 100644 --- a/packages/codemods/src/migrations/action-prop-to-on-action/transform.ts +++ b/packages/codemods/src/migrations/action-prop-to-on-action/transform.ts @@ -1,7 +1,8 @@ import type { Transform } from "jscodeshift"; /** - * Renames the `action` prop to `onAction` on `Action`. + * Renames the `action` prop to `onAction` on `Action`, and wraps a bare + * function reference passed to it in an inline call. * * The scope is deliberately narrow. Only JSX elements that resolve to `Action` * — imported (named or as a namespace) from `@mittwald/flow-react-components` @@ -14,6 +15,44 @@ import type { Transform } from "jscodeshift"; * An element that already carries `onAction` keeps it and only loses the stale * `action` prop, which mirrors what the runtime fallback does: an explicit * `onAction` wins. + * + * ## Why the wrap is unconditional + * + * `onAction` is typed `ActionFn` (`(...args: unknown[]) => unknown`), so a + * reference to a function declaring a parameter of any narrower type — the + * usual case being `controller.close`, typed `(options?: CloseOverlayOptions) + * => void` — does not type-check. Whether a given reference declares such a + * parameter needs type information and is not decidable from the source, which + * is why this transform used to leave every reference alone and `apply` asked + * for a manual pass over all of them. + * + * Deciding _whether_ to wrap needs the type. _Performing_ the wrap does not: + * `() => fn()` calls exactly what `fn` would have been called with by `Action`, + * minus the arguments — and `onAction` is documented as taking none. So the + * wrap fixes the reference that needed it and is a no-op for the rest, which + * makes the decision unnecessary. + * + * Two cases it can still change, neither decidable from the source: + * + * - A handler that reads the argument `Action` forwards (the trigger's event, + * e.g. a `PressEvent` from the `Button` inside) and declares it optional or + * as a rest parameter, so that it type-checks both before and after. It stops + * receiving that argument, silently. Nothing in Flow documents `onAction` as + * receiving one. + * - A possibly-undefined reference, `onAction={props.onAction}`. `undefined` is a + * valid value for the prop, but calling it is not, so the wrap turns + * something TypeScript accepted into something it rejects — loudly, at the + * call site, which is why this is a compile error to fix rather than a + * regression to find. + * + * `apply` names both. + * + * Only a bare reference is wrapped: a plain identifier or a member expression. + * An arrow function and a function expression already are the handler. A call + * (`makeHandler()`, `close.bind(controller)`) _produces_ the handler, so + * wrapping it would call the factory on every trigger and throw its result + * away. A conditional, a `??` chain or an optional member expression is not a + * reference to one function, so wrapping would be a guess. */ const actionPropToOnActionTransform: Transform = (fileInfo, { j }) => { const flowPackages = [ @@ -25,6 +64,30 @@ const actionPropToOnActionTransform: Transform = (fileInfo, { j }) => { const isFlowImport = (source: string): boolean => flowPackages.some((pkg) => source === pkg || source.startsWith(`${pkg}/`)); + /** + * Whether a member expression is rooted in a plain reference — + * `controller.close`, `this.handleSave`, `store.modal.controller.close` — + * rather than in something evaluated, as `makeController().close` is. + * + * An optional link disqualifies the chain. `controller?.close` parses as + * `OptionalMemberExpression` and never reaches here at all; walking the chain + * catches a mixed one like `(a?.b).c`. Declining is deliberate: `() => + * controller?.close()` short-circuits differently from the value it would + * replace, and the spelling says the reference may not be there. + */ + const hasPlainObjectChain = (expression: { object?: unknown }): boolean => { + const object = expression.object; + if (!object || typeof object !== "object" || !("type" in object)) { + return false; + } + const typed = object as { type: string; object?: unknown }; + return ( + typed.type === "ThisExpression" || + typed.type === "Identifier" || + (typed.type === "MemberExpression" && hasPlainObjectChain(typed)) + ); + }; + const root = j(fileInfo.source, { parser: "tsx" }); // Local JSX identifier -> canonical component name (resolves `as` aliases). @@ -94,13 +157,49 @@ const actionPropToOnActionTransform: Transform = (fileInfo, { j }) => { path.node.attributes = attributes.filter( (attribute) => !isNamed(attribute, "action"), ); - return; + } else { + for (const attribute of attributes) { + if (isNamed(attribute, "action") && attribute.type === "JSXAttribute") { + attribute.name.name = "onAction"; + } + } } - for (const attribute of attributes) { - if (isNamed(attribute, "action") && attribute.type === "JSXAttribute") { - attribute.name.name = "onAction"; + // Whatever the prop was called before, the surviving `onAction` is the one + // that needs the wrap — including one the consumer had already renamed by + // hand, which carries the same type error. + for (const attribute of path.node.attributes ?? []) { + if ( + attribute.type !== "JSXAttribute" || + !isNamed(attribute, "onAction") + ) { + continue; + } + + const container = attribute.value; + if (container?.type !== "JSXExpressionContainer") { + continue; } + + // A bare reference is an identifier or a member expression. Everything + // else either is the handler already or is a way of producing one. + const expression = container.expression; + if ( + expression.type !== "Identifier" && + expression.type !== "MemberExpression" + ) { + continue; + } + if ( + expression.type === "MemberExpression" && + !hasPlainObjectChain(expression) + ) { + continue; + } + + attribute.value = j.jsxExpressionContainer( + j.arrowFunctionExpression([], j.callExpression(expression, [])), + ); } }); diff --git a/packages/components/MIGRATION.md b/packages/components/MIGRATION.md index b53897fda3..9c00364428 100644 --- a/packages/components/MIGRATION.md +++ b/packages/components/MIGRATION.md @@ -1011,18 +1011,39 @@ 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. **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. +`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. ```shell npx @mittwald/flow-codemods@latest action-prop-to-on-action src