Actions: Fix HandlerFunction type to support async callback props#33864
Conversation
Change return type from void to any so action handlers can be assigned to async callback props without type errors.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/actions/models/HandlerFunction.ts (1)
1-1:anyreturn type is the correct pragmatic fix, but consider adding a type-level regression test.
voidis not assignable toPromise<void>in TypeScript's structural type system, and neither isunknown.anyis the only standard escape hatch that makesHandlerFunctionassignable to async callback props — the rationale is sound. It is also consistent with the pre-existingany[]parameter type.The one concern is that
anyturns off return-type checking for callers that capture the return value of aHandlerFunction. In practice this is benign (action handlers are side-effect-only), but it's worth guarding against future regressions by adding a type-level test — for example a.test-d.tsfile assertingHandlerFunctionis assignable to() => Promise<void>and() => void:import { expectTypeOf } from 'expect-type'; import type { HandlerFunction } from './HandlerFunction'; // Should be assignable to async callback props (the fixed case) expectTypeOf<HandlerFunction>().toMatchTypeOf<() => Promise<void>>(); // Should remain assignable to plain void callbacks expectTypeOf<HandlerFunction>().toMatchTypeOf<() => void>();As per coding guidelines, TypeScript strict mode should be enabled across all packages —
anyusage should be minimised where possible, but there is no viable stricter alternative in this specific case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/actions/models/HandlerFunction.ts` at line 1, Add a type-level regression test to lock in the `HandlerFunction` assignability: create a `.test-d.ts` file that imports `HandlerFunction` and uses `expectTypeOf` (or similar) to assert `HandlerFunction` is assignable to both `() => Promise<void>` and `() => void`; reference the `HandlerFunction` symbol in the assertions so future changes to `export type HandlerFunction = (...args: any[]) => any;` will fail the type test if regressions occur.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/actions/models/HandlerFunction.ts`:
- Line 1: Add a type-level regression test to lock in the `HandlerFunction`
assignability: create a `.test-d.ts` file that imports `HandlerFunction` and
uses `expectTypeOf` (or similar) to assert `HandlerFunction` is assignable to
both `() => Promise<void>` and `() => void`; reference the `HandlerFunction`
symbol in the assertions so future changes to `export type HandlerFunction =
(...args: any[]) => any;` will fail the type test if regressions occur.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/core/src/actions/models/HandlerFunction.test-d.ts (1)
5-8:toMatchTypeOfis deprecated — usetoExtendinstead.
toMatchTypeOfhas been deprecated sinceexpect-typev1.2.0; the replacement istoExtend.♻️ Proposed fix
-expectTypeOf<HandlerFunction>().toMatchTypeOf<() => Promise<void>>(); +expectTypeOf<HandlerFunction>().toExtend<() => Promise<void>>(); -expectTypeOf<HandlerFunction>().toMatchTypeOf<() => void>(); +expectTypeOf<HandlerFunction>().toExtend<() => void>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/actions/models/HandlerFunction.test-d.ts` around lines 5 - 8, Replace deprecated expect-type matcher toMatchTypeOf with the new toExtend matcher for the type assertions involving HandlerFunction; specifically update the assertions that call expectTypeOf<HandlerFunction>().toMatchTypeOf<() => Promise<void>>() and expectTypeOf<HandlerFunction>().toMatchTypeOf<() => void>() to use toExtend instead, keeping the same generic type arguments and intent so the tests continue to assert assignability to both async and plain void callbacks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/actions/models/HandlerFunction.test-d.ts`:
- Line 1: Replace the deprecated expect-type import and matcher: change the
import of expectTypeOf in HandlerFunction.test-d.ts to come from 'vitest'
instead of 'expect-type', and update any usages of the old matcher toMatchTypeOf
to the new toExtend matcher (e.g., calls like
expectTypeOf(<type>).toMatchTypeOf(...) should become
expectTypeOf(<type>).toExtend(...)); ensure all occurrences in this test file
are updated so the file follows the same pattern as other type-test files.
---
Nitpick comments:
In `@code/core/src/actions/models/HandlerFunction.test-d.ts`:
- Around line 5-8: Replace deprecated expect-type matcher toMatchTypeOf with the
new toExtend matcher for the type assertions involving HandlerFunction;
specifically update the assertions that call
expectTypeOf<HandlerFunction>().toMatchTypeOf<() => Promise<void>>() and
expectTypeOf<HandlerFunction>().toMatchTypeOf<() => void>() to use toExtend
instead, keeping the same generic type arguments and intent so the tests
continue to assert assignability to both async and plain void callbacks.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 2 | 2 | 0 |
| Self size | 402 KB | 391 KB | 🎉 -11 KB 🎉 |
| Dependency size | 338 KB | 338 KB | 🎉 -3 B 🎉 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 49 | 49 | 0 |
| Self size | 20.20 MB | 20.41 MB | 🚨 +211 KB 🚨 |
| Dependency size | 16.52 MB | 16.52 MB | 🎉 -2 B 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 534 | 534 | 0 |
| Self size | 648 KB | 648 KB | 🎉 -404 B 🎉 |
| Dependency size | 59.86 MB | 59.89 MB | 🚨 +35 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 92 | 92 | 0 |
| Self size | 1.12 MB | 1.12 MB | 🎉 -5 B 🎉 |
| Dependency size | 22.47 MB | 22.50 MB | 🚨 +35 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 124 | 124 | 0 |
| Self size | 30 KB | 30 KB | 🎉 -57 B 🎉 |
| Dependency size | 23.76 MB | 23.79 MB | 🚨 +35 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 82 | 82 | 0 |
| Self size | 35 KB | 35 KB | 🎉 -22 B 🎉 |
| Dependency size | 20.25 MB | 20.29 MB | 🚨 +35 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 271 | 271 | 0 |
| Self size | 24 KB | 24 KB | 🚨 +7 B 🚨 |
| Dependency size | 44.53 MB | 44.56 MB | 🚨 +35 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 183 | 183 | 0 |
| Self size | 779 KB | 779 KB | 🚨 +83 B 🚨 |
| Dependency size | 67.38 MB | 67.59 MB | 🚨 +211 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 176 | 176 | 0 |
| Self size | 32 KB | 32 KB | 🚨 +34 B 🚨 |
| Dependency size | 65.90 MB | 66.11 MB | 🚨 +211 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 1.04 MB | 1.04 MB | 🎉 -165 B 🎉 |
| Dependency size | 36.72 MB | 36.93 MB | 🚨 +211 KB 🚨 |
| Bundle Size Analyzer | node | node |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 58 | 57 | 🎉 -1 🎉 |
| Self size | 1.19 MB | 1.23 MB | 🚨 +36 KB 🚨 |
| Dependency size | 13.21 MB | 12.94 MB | 🎉 -264 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/actions/models/HandlerFunction.test-d.ts (1)
4-8: Consider wrapping assertions in namedtest()blocks for cleaner reporter output.The top-level assertions are valid —
*.test-d.tsfiles are not run by Vitest, they are only statically analyzed by the compiler. However, namedtest()blocks make failures easier to identify in CI output and align with the pattern in other type-test files in the repo.Optionally, adding a
not.toExtendnegative assertion for() => voidreturn type would document the regression boundary and confirm the old behaviour was incompatible.✨ Optional: named test blocks + negative regression guard
+import { test } from 'vitest'; import { expectTypeOf } from 'vitest'; import type { HandlerFunction } from './HandlerFunction'; -// Should be assignable to async callback props (the fixed case) -expectTypeOf<HandlerFunction>().toExtend<() => Promise<void>>(); - -// Should remain assignable to plain void callbacks -expectTypeOf<HandlerFunction>().toExtend<() => void>(); +test('HandlerFunction is assignable to async callback props', () => { + expectTypeOf<HandlerFunction>().toExtend<() => Promise<void>>(); +}); + +test('HandlerFunction remains assignable to plain void callbacks', () => { + expectTypeOf<HandlerFunction>().toExtend<() => void>(); +}); + +test('HandlerFunction typed as void-only is NOT assignable to async callbacks (regression guard)', () => { + // A (...args: any[]) => void should not extend () => Promise<void> + expectTypeOf<(...args: any[]) => void>().not.toExtend<() => Promise<void>>(); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/actions/models/HandlerFunction.test-d.ts` around lines 4 - 8, Wrap the top-level type assertions for HandlerFunction in named test() blocks so test reporters show clear failure locations: create one test() with a name like "HandlerFunction is assignable to async callback" that runs expectTypeOf<HandlerFunction>().toExtend<() => Promise<void>>(), and another named test() like "HandlerFunction remains assignable to void callback" that runs expectTypeOf<HandlerFunction>().toExtend<() => void>(); optionally add a negative regression guard test using expectTypeOf<HandlerFunction>().not.toExtend<() => void>() to document the prior incompatible behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/actions/models/HandlerFunction.test-d.ts`:
- Around line 4-8: Wrap the top-level type assertions for HandlerFunction in
named test() blocks so test reporters show clear failure locations: create one
test() with a name like "HandlerFunction is assignable to async callback" that
runs expectTypeOf<HandlerFunction>().toExtend<() => Promise<void>>(), and
another named test() like "HandlerFunction remains assignable to void callback"
that runs expectTypeOf<HandlerFunction>().toExtend<() => void>(); optionally add
a negative regression guard test using
expectTypeOf<HandlerFunction>().not.toExtend<() => void>() to document the prior
incompatible behavior.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
code/core/src/actions/models/HandlerFunction.ts (1)
1-1:⚠️ Potential issue | 🟠 Major
void | Promise<void>return type is not assignable toPromise<void>in strict TypeScriptThe current type on Line 1 cannot be assigned to callback props expecting
() => Promise<void>, leaving issue#23731unresolved. To support the stated objective of allowing async callbacks without errors, use a fully permissive return type.Proposed fix
-export type HandlerFunction = (...args: any[]) => void | Promise<void>; +export type HandlerFunction = (...args: any[]) => any;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/actions/models/HandlerFunction.ts` at line 1, The HandlerFunction type's return signature is too strict; change export type HandlerFunction = (...args: any[]) => void | Promise<void>; to a fully permissive return type so async callbacks can be assigned to Promise-returning callbacks. Locate HandlerFunction and update its return to a permissive type such as (...args: any[]) => any | Promise<any> (or use unknown/Promise<unknown> if preferred) so synchronous void and async Promise results are both assignable to () => Promise<void>.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@code/core/src/actions/models/HandlerFunction.ts`:
- Line 1: The HandlerFunction type's return signature is too strict; change
export type HandlerFunction = (...args: any[]) => void | Promise<void>; to a
fully permissive return type so async callbacks can be assigned to
Promise-returning callbacks. Locate HandlerFunction and update its return to a
permissive type such as (...args: any[]) => any | Promise<any> (or use
unknown/Promise<unknown> if preferred) so synchronous void and async Promise
results are both assignable to () => Promise<void>.
Closes #23731
What I did
Changed the return type of
HandlerFunctionfromvoidtoanyso that action handlers can be assigned to async callback props (e.g.onSubmit: () => Promise<void>) without causing type errors.The issue is that
(...args: any[]) => voidis not assignable to(...args: any[]) => Promise<void>, but(...args: any[]) => anyis, sinceanyis compatible with all return types.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This is a type-only change. Verified that the existing
HandlerFunctionusages in the codebase (e.g.action()return type,ActionsMap, test stories) remain compatible.Summary by CodeRabbit
Updates
Tests