Repository navigation
Conversation
The handler recognised a useCallback-wrapped property as a method, but the documentation was built from the call expression, so the method came out with no parameters and no return type. resolveToMethodFunction now resolves a value to the function it documents, unwrapping a React useCallback call, and both the handler's method test and the documentation builder use it. The tests assert the wrapped function's signature and cover a local function named useCallback, a call with no arguments and a non-function argument.
The handler's unwrapping now runs for every method node path, so add the surfaces it reaches beyond the imperative handle identifier: a callback written inline in the handle object, an ObjectExpression component, a statics object, a class property and a Component.foo assignment, plus the docblock and the async/generator modifiers of the wrapped function, a renamed useCallback import and a handle that precedes the declaration. The controls pin the guards the unwrapping keeps: a local useCallback, a missing or non-function argument, a spread argument, a nested call and useMemo.
Every positive callback fixture carried a typed parameter and a return
annotation, so the empty-signature path the boundary ledger claims was
never exercised. Admit useCallback(() => {}, []) through
useImperativeHandle and assert an empty params list and a null return.
🦋 Changeset detectedLatest commit: 1d45d98 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for react-docgen ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Thanks for the PR.
The core useImperativeHandle + useCallback support makes sense, but I think the tests and implementation should be tightened before merging. In particular, please cover the exact issue reproducer and either narrow or consistently implement the additional method surfaces.
| ])( | ||
| 'extracts a method wrapped with $name', | ||
| ({ imports, setup, value, imperativeHandle }) => { | ||
| const definition = parse.statementLast<FunctionDeclaration>(` |
There was a problem hiding this comment.
Could we add a regression test that runs the exact shape from #856 through the normal parser/resolver: memo(forwardRef(...)), a top-level useCallback, and shorthand exposure through useImperativeHandle? These tests invoke the handler directly with a plain function, so they do not pin the wrapper-resolution path from the reported bug.
AI disclosure: This comment was added by ChatGPT 5.6 Sol.
There was a problem hiding this comment.
Added in c97359c: src/__tests__/imperativeHandleCallbacks-test.ts parses the component from #856 verbatim (memo(forwardRef(...)), a top-level useCallback, shorthand exposure through useImperativeHandle) through the public parse(), alongside a plain-function memo(forwardRef) case. Both fail on main without the fix.
|
|
||
| isProbablyMethod = value.isFunction(); | ||
| isProbablyMethod = | ||
| resolveToMethodFunction(path.get('value') as NodePath) !== null; |
There was a problem hiding this comment.
This changes generic object and class-property method detection, although #856 only asks about methods exposed through useImperativeHandle. It also adds support for useCallback in class fields/statics, where Hooks are not valid React usage. Could we keep the unwrapping local to the imperative-handle path unless there is a separate use case for broadening every method surface?
AI disclosure: This comment was added by ChatGPT 5.6 Sol.
There was a problem hiding this comment.
Narrowed in f5d7939. The useCallback unwrapping now lives only on the imperative-handle path: getMethodFunctionExpression keeps its old plain-function resolution and consults useCallback only when the caller passes isImperativeHandle, which the handler sets from the useImperativeHandle visitor alone. Class fields, statics and object methods are back to base behaviour, and there are tests pinning that they stay there.
| .statementLast<ExpressionStatement>( | ||
| `import { useCallback } from 'react'; | ||
| const Foo = () => {} | ||
| Foo.foo = useCallback((bar: number): number => bar, []) |
There was a problem hiding this comment.
This test suggests that Component.foo = useCallback(...) is supported, but findAssignedMethods still filters assignments with resolveToValue(right).isFunction(), so the public handler never passes this assignment to getMethodDocumentation. Please either update the handler consistently and test it through componentMethodsHandler, or remove this out-of-scope assignment case.
AI disclosure: This comment was added by ChatGPT 5.6 Sol.
There was a problem hiding this comment.
Removed in e9a51c5. The Component.foo = useCallback(...) case was out of scope and the handler never reached it; the test file is gone rather than the handler widened.
The handler tests build a component definition and call the handler directly, so none of them exercise the resolver path the issue reports: a memo(forwardRef) component whose method is exposed as a shorthand property. Parse that component verbatim through the public parse() and assert the documented method, with the same component without useCallback as a control for memo/forwardRef resolution.
Documenting the function a useCallback call wraps changed every method surface, including class fields and statics objects where hooks are not valid React usage. Pass the imperative handle down to getMethodDocumentation and unwrap only there; the other surfaces keep documenting plain function values only. The surface tests become controls: they assert that an object, statics and class-property useCallback stays undocumented, next to a plain function on the same surface that still is.
findAssignedMethods only collects assignments whose right-hand side resolves to a function, so Component.foo = useCallback(...) never reaches getMethodDocumentation through the handler. Remove the unit test that implied the assignment surface supported it.
The imperative handle admission keeps the React lifecycle check, so a handle property named after a lifecycle method stays undocumented even when its value is a useCallback call. Control: it passes with and without the unwrapping.
|
Thanks for the review. Pushed a9605a4..1d45d98, one commit per point: the exact #856 shape now runs through the parser (c97359c), the unwrapping is confined to the imperative-handle path (f5d7939), and the assignment case is dropped (e9a51c5); 1d45d98 pins the lifecycle filter on an unwrapped callback. Package suite: 1467 tests pass, prettier, eslint and tsc clean. |
Fixes #856
Methods exposed through
useImperativeHandlethat are wrapped inuseCallbackare now documented with the wrapped function's signature, docblock and async/generator modifiers. On main the component from #856 yieldsmethods: [].The unwrapping is limited to the imperative-handle path. Class fields, statics objects and object-literal components keep their current behaviour.
Review follow-ups from 2026-09-21, one commit each:
parse()(memo(forwardRef(...)), top-level useCallback, shorthand property).options.isImperativeHandleis set, which only the imperative-handle visitor sets.Component.foo = useCallback(...)test was removed.Tests: 12 new behavioural tests fail on main and pass here, 13 controls pass on both. The full package suite, prettier, eslint and tsc are clean.
AI tooling helped find and draft this; I ran the tests and own the change.