Add TypeScript type definitions from DefinitelyTyped#12
Conversation
WalkthroughAdds a complete TypeScript declaration surface for the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
|
@claude review this PR |
|
Claude finished @justin808's task in 1m 26s —— View job PR Review: TypeScript Type Definitions
SummaryThis PR successfully adds TypeScript type definitions from DefinitelyTyped to the recompose package. The implementation is solid overall, with the types correctly copied from the Issues Found1. Critical: Missing exports in type definitions (src/packages/recompose/index.d.ts)The type definitions declare Impact: Users will see type definitions suggesting these functions exist, but they'll get runtime errors when trying to use them. Recommendation: Either remove these type declarations from the 2. Typo: "TOutter" instead of "TOuter" (throughout the file)The type definitions consistently use
Impact: Low - this is a cosmetic issue and doesn't affect functionality, but it's inconsistent with TypeScript naming conventions. Recommendation: Consider renaming all instances to 3. Incorrect withState type signature (line 126)The & { [stateUpdateName in TStateUpdaterName]: (state: TState) => TState };This suggests the state updater returns Impact: Users won't be able to properly type check their state updater calls. Recommendation: The type should be: & { [stateUpdateName in TStateUpdaterName]: (state: TState | ((prevState: TState) => TState), callback?: () => void) => void };Positive Aspects✅ Package.json correctly configured with Test Plan VerificationThe test plan includes:
This is a good test, but I recommend also testing:
RecommendationBEFORE MERGING: Fix issue #1 (remove Issues #2 and #3 could be addressed in follow-up PRs if desired, as they're lower priority. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/packages/recompose/index.d.ts`:
- Around line 423-427: The type ObservableConfig is declared but not exported,
causing imports (e.g., in observable-config submodules) to fail; export the type
by adding an export for ObservableConfig so external modules can import it
(ensure the declaration for ObservableConfig is changed to an exported type and
keep the existing setObservableConfig signature referring to that exported
type); update any related declarations that import ObservableConfig to use the
exported symbol as-is.
- Around line 476-482: Add a missing module declaration for
"@shakacode/recompose/flydObservableConfig" matching the pattern used by other
observable configs (e.g., the kefir declaration): import the ObservableConfig
type, declare a const flydConfig: ObservableConfig, and export it as the default
export (i.e., export default flydConfig) so the type definition for the flyd
config exists alongside the other observable modules.
- Around line 345-354: Update the type signatures to match the JS
implementation: change getDisplayName to accept Component<any> | string |
undefined and to return string | undefined (so it returns undefined for falsy
input and passes through string inputs), and change wrapDisplayName to accept
Component<any> | string | undefined (so callers can pass a string component
name) while keeping its return type as string; update the exported function
declarations for getDisplayName and wrapDisplayName accordingly.
🧹 Nitpick comments (2)
docs/API.md (1)
898-908: Documentation uses higher-kinded type notation (TStream<T>) that isn't valid TypeScript.The
componentFromStreamWithConfig,mapPropsStreamWithConfig,createEventHandlerWithConfig, andsetObservableConfigsignatures in the docs useTStream<T>as a generic type constructor (e.g.,(observable: Observable<T>) => TStream<T>). TypeScript doesn't support higher-kinded types, and the actualindex.d.tscorrectly works around this withany. Since this is API documentation meant for human consumption, this is acceptable as pseudocode, but it could confuse TypeScript users who try to use these signatures literally.Consider adding a brief note (e.g., "Signatures below use pseudocode; see the
.d.tsfile for exact TypeScript types") near the Observable utilities section header.Also applies to: 977-988, 1019-1031, 1037-1042
src/packages/recompose/index.d.ts (1)
382-386:hoistStaticssignature is overly restrictive — only acceptsInferableComponentEnhancer.The current overload only works with
InferableComponentEnhancer<TProps>, buthoistStaticsis commonly used with any HOC includingInferableComponentEnhancerWithPropsandComponentEnhancer. This will force unnecessary type assertions at call sites.Proposed overloads
export function hoistStatics<TProps>( hoc: InferableComponentEnhancer<TProps>, blacklist?: { [key: string]: boolean }, ): InferableComponentEnhancer<TProps>; + export function hoistStatics<TInner, TOuter>( + hoc: InferableComponentEnhancerWithProps<TInner, TOuter>, + blacklist?: { [key: string]: boolean }, + ): InferableComponentEnhancerWithProps<TInner, TOuter>;
| // getDisplayName: https://github.com/shakacode/recompose/blob/master/docs/API.md#getDisplayName | ||
| export function getDisplayName( | ||
| component: Component<any>, | ||
| ): string; | ||
|
|
||
| // wrapDisplayName: https://github.com/shakacode/recompose/blob/master/docs/API.md#wrapDisplayName | ||
| export function wrapDisplayName( | ||
| component: Component<any>, | ||
| wrapperName: string, | ||
| ): string; |
There was a problem hiding this comment.
getDisplayName and wrapDisplayName types don't match the JS implementation.
Per getDisplayName.js, the function accepts string values (returns them as-is) and returns undefined for falsy input. The current type only accepts Component<any> and always returns string, which will cause type errors for valid call sites like getDisplayName('div').
Similarly, wrapDisplayName accepts a string component name per the implementation.
Proposed fix
export function getDisplayName(
- component: Component<any>,
+ component: Component<any> | string,
): string;
export function wrapDisplayName(
- component: Component<any>,
+ component: Component<any> | string,
wrapperName: string,
): string;🤖 Prompt for AI Agents
In `@src/packages/recompose/index.d.ts` around lines 345 - 354, Update the type
signatures to match the JS implementation: change getDisplayName to accept
Component<any> | string | undefined and to return string | undefined (so it
returns undefined for falsy input and passes through string inputs), and change
wrapDisplayName to accept Component<any> | string | undefined (so callers can
pass a string component name) while keeping its return type as string; update
the exported function declarations for getDisplayName and wrapDisplayName
accordingly.
Copy index.d.ts from @types/shakacode__recompose into the package so types ship with the library instead of requiring a separate @types install. Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Remove createEagerElement and createEagerFactory (not in JS exports) - Remove custom Omit type (built into TypeScript since 3.5) - Fix TOutter/outterName typos to TOuter/outerName throughout - Fix withState updater type to accept value or function, return void - Fix branch to share TInner/TOuter across both enhancers - Fix renderComponent return type to preserve TProps - Fix withContext getChildContext return type to use TContext - Make createSink generic over TProps Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Convert Flow syntax (?Type, ?param) to TypeScript (Type?, param?) - Add descriptive type parameters (TState, TAction, TStream, TValue) with explanatory comments to all generic signatures - Add type parameters to withState, withStateHandlers, withContext, createSink to match withReducer/createEventHandler pattern - Fix withReducer dispatch callback type to receive new state - Change code blocks from js to ts for typed signatures Co-Authored-By: Claude Opus 4.6 <[email protected]>
206dc84 to
70e74e0
Compare
|
@coderabbitai Review. |
|
@claude Review. |
|
✅ Actions performedReview triggered.
|
|
Claude finished @alexeyr-ci2's task in 1m 39s —— View job PR Review
SummaryThis PR successfully adds TypeScript type definitions from DefinitelyTyped to the recompose package. The implementation is solid and all critical issues from CodeRabbit's previous review have been addressed in commits Issues Previously Raised and ResolvedAll three actionable comments from CodeRabbit's latest review have been fixed:
Current State ReviewAfter reviewing the current state of the code, I found the implementation to be correct and well-structured: Strengths:
Verified Type Accuracy:
Minor Observations (Non-blocking)These are cosmetic and inherited from DefinitelyTyped - not issues that need fixing:
Recommendation✅ APPROVE - This PR is ready to merge. All critical issues have been addressed, type definitions accurately match the JavaScript implementation, and the package is properly configured to ship types with the library. Test PlanThe test plan includes:
I recommend also testing:
|
Summary
index.d.tsfrom @types/shakacode__recompose into the package so types ship with the library"types": "index.d.ts"to package.jsonTest plan
@types/shakacode__recompose🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores