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. |
2 participants
Fixes #856
Summary
useImperativeHandlemethods that are wrapped inuseCallbackare now documented, with the signature, docblock and modifiers of the function the callback wraps.parse(), so the wrapper-resolution path (memo(forwardRef(...))+ shorthand property) is pinned end to end, not just the handler.Current head
1d45d98. Review feedback from the 2026-09-21 review is addressed in three commits (one per comment) plus one follow-up control; see Review follow-ups below.Against
main(4abfaaf2) with the same test files in place, exactly the 12 behavioural tests fail and all 13 controls pass:The reported component itself, before and after:
Review follow-ups
c97359csrc/__tests__/imperativeHandleCallbacks-test.tsparses the issue's component verbatim via the publicparse()—memo(forwardRef(...)), top-leveluseCallback, shorthand exposure — plus aforwardRef-only case and a plain-functionmemo(forwardRef)control.f5d7939resolveToMethodFunctionbecameresolveToUseCallbackFunction(only theuseCallbackbranch).getMethodFunctionExpressionkeeps its original plain-function resolution and consultsuseCallbackonly whenoptions.isImperativeHandleis set, which onlyfindImperativeHandleMethodssets.isMethodis restored to its original body; the handler's imperative-handle visitor uses a newisImperativeHandleMethod. Class fields, statics and object components behave exactly as onmain.Component.foo = useCallback(...)casee9a51c5src/utils/__tests__/getMethodDocumentation-useCallback-test.tsdeleted.findAssignedMethodsstill filters onresolveToValue(right).isFunction(), so that assignment never reachesgetMethodDocumentation, and nothing in the change claims it does.1d45d98Tests on the branch
Behavioural tests fail on
main(4abfaaf2) and pass here. Controls pass on both arms; each states what it controls for.memo/forwardRefresolution, so the two above fail on the unwrappingisReactComponentMethodstill filters an unwrapped callbacka9605a4, passes onmainand herea9605a4a9605a4arguments[0]tolerates an empty listisFunctionon the resolved argument!Array.isArray(callback)guarduseCallback, not every builtin25 tests across the two files: 12 fail on
main, 13 are controls.src/utils/__tests__/getMethodDocumentation-test.ts(25) andsrc/handlers/__tests__/componentMethodsHandler-test.ts(25) are the pre-existing suites for the touched files and stay green.Boundaries
Every predicate and index expression the diff adds or changes:
options.isImperativeHandleingetMethodFunctionExpressionuseCallbackfallback runs; unset → identical tomaina9605a4)functionExpression.isFunction()(unchanged first branch)function, non-function valuevalue.isCallExpression()42)isReactBuiltinCall(value, 'useCallback')useCallback,React.useCallback, renamed import, local binding of the same name,useMemouseCallbackbinding unwrapsvalue.get('arguments')[0]callback && !Array.isArray(callback)undefined, single path, array (spread)wrapped.isFunction()afterresolveToValueuseCallbackcallpath.isObjectProperty()inisImperativeHandleMethodisMethod; a spread is neither and is skippedisMethodbranch covered by "documents a plain function exposed through the handle (control)"; a spread has novalueand returns false — no separate test!isReactComponentMethod(path)inisImperativeHandleMethod() => {}(no params, no return annotation)params: [],returns: nulluseImperativeHandlecallforwardRef,memo(forwardRef(...))imperativeHandleCallbackstestsDecisions
The unwrapping goes through
options.isImperativeHandlerather than a separate resolver call in the handler so that params, returns, docblock and the async/generator modifiers all come from one resolution of the same function; an earlier revision unwrapped insideisMethodonly, which admitted the method but documented theuseCallbackcall's own (empty) signature. OneuseCallbacklayer is unwrapped, not recursively, and no other React builtin is treated as a method wrapper.Checks run locally: whole
react-docgenpackage suite (67 files, 1468 tests),prettier --check,eslint --report-unused-disable-directives --max-warnings=0, andtsc --noEmiton the package tsconfig — all clean.AI assistance: this bug was found and the fix and tests were drafted with AI tooling in my workflow; the tests and checks above were executed as pasted. I'm responsible for the change and will handle review feedback.