Skip to content

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

Open
mfal wants to merge 2 commits into
mainfrom
fix/codemod-action-wrap-bare-reference-3060
Open

fix(codemods): wrap bare function references passed to onAction#3075
mfal wants to merge 2 commits into
mainfrom
fix/codemod-action-wrap-bare-reference-3060

Conversation

@mfal

@mfal mfal commented Sep 2, 2026

Copy link
Copy Markdown
Member

action-prop-to-on-action renamed the prop and left the value alone, so its
apply asked for a manual pass over every call site. That instruction was
answering the wrong question.

Deciding whether a reference needs wrapping needs type information — whether
controller.close declares a parameter is not in the source. Performing the
wrap does not: () => fn() calls exactly 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 removes the decision instead of
answering it.

What the codemod does now

Wrapped — a bare reference:

  • a plain identifier, action={close}onAction={() => close()}
  • a member expression rooted in an identifier or this,
    action={controller.close}, action={store.modal.controller.close},
    action={this.handleSave}

Left alone:

  • an arrow function and a function expression — already the handler
  • a call: makeHandler(), close.bind(controller), useCallback(close, []).
    These produce the handler, so wrapping would call the factory on every
    trigger and throw its result away. This is the one shape where a naive
    "wrap everything" is plainly wrong
  • anything that is not one reference: isOpen ? close : open,
    onClose ?? noop, controller?.close

The wrap targets the surviving onAction whatever the prop was called before,
so a consumer who renamed by hand — or ran the previous version of this codemod
— gets the wrap on a second run. That also keeps it idempotent: its own output
is an arrow function, which it skips.

Where unconditional wrapping is not a no-op

Two cases, neither decidable from the source. apply and the entry body name
both.

  1. A handler that reads the argument Action forwards. Action does
    forward the trigger's event — Button.onPressaction.execute(...args)
    ActionExecutionBatch.executeBatch(...args) → the handler. A handler
    declaring that parameter as optional or rest type-checks both before and
    after the wrap, so it silently stops receiving the event. Nothing documents
    onAction as receiving an argument (ActionProps.onAction's JSDoc says only
    "the function executed when the action is triggered"), and no Flow-internal
    handler reads one — but a consumer relying on it gets no warning. This is the
    only silent case I found.
  2. A possibly-undefined reference, onAction={props.onAction}. undefined
    is a valid value for the prop; calling it is not. The wrap turns something
    TypeScript accepted into something it rejects — loudly, at the call site, so
    it is a compile error to fix rather than a regression to find.

entry.md

The body's closing paragraph claimed the codemod "deliberately does not wrap",
and apply told the reader (or agent) to check every site by hand. Both
rewritten to what the codemod now does, plus the two cases above. MIGRATION.md
and migrations.generated.ts regenerated from it.

Verification

  • pnpm nx test:unit codemods — 319 pass, 13 of them this transform's fixtures
    (12 new: bare identifier, member expression, deep chain, this, a value
    already spelled onAction, arrow, function expression, call, conditional /
    ?? / optional chain, alias + namespace import, non-Flow Action and a plain
    <form action>, sibling props)
  • pnpm nx test:compile codemods clean
  • pnpm lint clean (0 errors; the 71 + 118 warnings are pre-existing)

Overlap with #3047

#3047 (sibling issue #3061) touches the same two generated files,
packages/codemods/src/migrations.generated.ts and
packages/components/MIGRATION.md, for different entries
(renamed-css-export, table-column-width-props) and different hunks. No
source file is shared. Whichever merges second may need
pnpm nx build codemods re-run and the two artifacts recommitted.

fixes #3060

🤖 Generated with Claude Code

@mfal mfal self-assigned this Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Coverage Report for ./packages/components/

Status Category Percentage Covered / Total
🔵 Lines 78.69% 746 / 948
🔵 Statements 78.57% 763 / 971
🔵 Functions 80.09% 165 / 206
🔵 Branches 70.33% 377 / 536
File CoverageNo changed files found.
Generated in workflow #6616 for commit 9068c68 by the Vitest Coverage Report Action

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

🚀 Preview Deployment

Preview environments are ready:

Type URL
docs pr-3075.docs.review.flow-components.de
storybook pr-3075.storybook.review.flow-components.de

Images:

  • docs: ghcr.io/mittwald/flow/docs:pr-3075
  • storybook: ghcr.io/mittwald/flow/storybook:pr-3075

`action-prop-to-on-action` renamed the prop and left the value alone, so
`apply` asked for a manual pass over every call site. Deciding *whether* a
reference needs wrapping needs type information. Performing the wrap does not:
`() => 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 removes the decision.

Wrapped: a plain identifier and a member expression rooted in one, `this`
included. Left alone: an arrow function and a function expression (already the
handler), a call (`makeHandler()`, `close.bind(controller)` — it produces the
handler), and anything that is not one reference (`isOpen ? close : open`,
`onClose ?? noop`, `controller?.close`).

The wrap is applied to the surviving `onAction` whatever the prop was called
before, so a consumer who renamed by hand gets it too.

Two cases it changes and `apply` now names: a handler reading the event
`Action` forwards (undocumented, but forwarded) stops receiving it, and
`onAction={props.onAction}` becomes a call TypeScript rejects — loud, at the
call site.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mfal
mfal force-pushed the fix/codemod-action-wrap-bare-reference-3060 branch from 4de21cd to 9d29d86 Compare September 2, 2026 12:50
@mfal
mfal marked this pull request as ready for review September 3, 2026 12:22
@mfal
mfal requested a review from a team September 3, 2026 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

codemods: action-prop-to-on-action could auto-wrap bare function references

1 participant