-
Notifications
You must be signed in to change notification settings - Fork 0
Release v1.1.0: Add useAsyncMemoSuspense hook with implementation, tests, and documentation #7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ompatibility tests
…ng in useAsyncMemo and useAsyncEffekt
feat: add test suite with GitHub Actions workflow and React version compatibility tests
Adds JSDocs, README documentation, and tests for the new `useAsyncMemoSuspense` hook. The documentation includes examples and important notes about SSR and client component usage. The tests cover suspense, error handling, dependency changes, and scope usage.
…ithChildren syntax
Feat/add use async memo suspense
WalkthroughA new experimental React hook, Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant useAsyncMemoSuspense
participant Cache
participant Promise
participant ReactSuspense
Component->>useAsyncMemoSuspense: Call with factory, deps, options
useAsyncMemoSuspense->>Cache: Check for cache entry (keyed by factory+deps+scope)
alt Cache miss or deps changed
useAsyncMemoSuspense->>Promise: Invoke async factory
useAsyncMemoSuspense->>Cache: Store pending entry with promise
useAsyncMemoSuspense-->>Component: throw Promise (suspends)
Component->>ReactSuspense: Suspended until promise resolves
Promise-->>Cache: Resolve or reject
ReactSuspense->>Component: Resume rendering with result or error
else Cache hit (success)
useAsyncMemoSuspense-->>Component: Return cached value
else Cache hit (error)
useAsyncMemoSuspense-->>Component: throw cached error
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/test.yml (1)
44-50: Consider improving the test exclusion strategy.The conditional test execution is appropriate for React version compatibility, but the current implementation could be improved:
- The command on line 46 is complex and hard to maintain
- The regex pattern
.*useAsyncMemoSuspense.*might be too broad- Coverage exclusion is duplicated between the workflow and jest config
Consider these improvements:
- - name: Run tests (excluding Suspense tests for React 16/17) - if: matrix.react-version == '16.8.0' || matrix.react-version == '17.0.0' - run: npm test -- --coverage --watchAll=false --testPathIgnorePatterns=".*useAsyncMemoSuspense.*" --collectCoverageFrom="src/**/*.ts" --collectCoverageFrom="!src/useAsyncMemoSuspense.ts" + - name: Run tests (excluding Suspense tests for React 16/17) + if: matrix.react-version == '16.8.0' || matrix.react-version == '17.0.0' + run: npm test -- --coverage --watchAll=false --testPathIgnorePatterns="useAsyncMemoSuspense\.test\.(ts|tsx)$"This approach:
- Uses a more precise regex pattern
- Removes coverage exclusion duplication (handled by jest config)
- Simplifies the command line
README.md (1)
17-17: Consider more professional wording.The "vibe coded" terminology might be too informal for a professional library documentation. Consider a more professional alternative.
-Note: Tests are vibe coded. Specific tests are added when bugs are reported. +Note: Tests are behavior-driven. Additional tests are added when bugs are reported.src/useAsyncMemoSuspense.ts (1)
12-18: Consider memory management for the global cacheThe global cache can grow unbounded over time, potentially causing memory issues in long-running applications. Consider implementing a cache eviction strategy (LRU, TTL, or max size limit).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.github/workflows/test.yml(1 hunks)README.md(4 hunks)package.json(3 hunks)src/__tests__/useAsyncMemo.test.ts(1 hunks)src/__tests__/useAsyncMemoSuspense.test.tsx(1 hunks)src/index.ts(1 hunks)src/useAsyncMemoSuspense.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: davemecha
PR: davemecha/use-async-effekt#3
File: src/__tests__/test-utils.ts:6-8
Timestamp: 2025-06-29T11:06:37.033Z
Learning: In matrix testing scenarios where different versions of testing libraries are dynamically installed (like @testing-library/react-hooks vs @testing-library/react across React versions), using `any` types may be necessary due to unstable type dependencies, rather than importing specific types that may not be available in all test configurations.
src/__tests__/useAsyncMemo.test.ts (1)
Learnt from: davemecha
PR: davemecha/use-async-effekt#3
File: src/__tests__/test-utils.ts:6-8
Timestamp: 2025-06-29T11:06:37.033Z
Learning: In matrix testing scenarios where different versions of testing libraries are dynamically installed (like @testing-library/react-hooks vs @testing-library/react across React versions), using `any` types may be necessary due to unstable type dependencies, rather than importing specific types that may not be available in all test configurations.
.github/workflows/test.yml (1)
Learnt from: davemecha
PR: davemecha/use-async-effekt#3
File: src/__tests__/test-utils.ts:6-8
Timestamp: 2025-06-29T11:06:37.033Z
Learning: In matrix testing scenarios where different versions of testing libraries are dynamically installed (like @testing-library/react-hooks vs @testing-library/react across React versions), using `any` types may be necessary due to unstable type dependencies, rather than importing specific types that may not be available in all test configurations.
package.json (1)
Learnt from: davemecha
PR: davemecha/use-async-effekt#3
File: src/__tests__/test-utils.ts:6-8
Timestamp: 2025-06-29T11:06:37.033Z
Learning: In matrix testing scenarios where different versions of testing libraries are dynamically installed (like @testing-library/react-hooks vs @testing-library/react across React versions), using `any` types may be necessary due to unstable type dependencies, rather than importing specific types that may not be available in all test configurations.
README.md (1)
Learnt from: davemecha
PR: davemecha/use-async-effekt#3
File: src/__tests__/test-utils.ts:6-8
Timestamp: 2025-06-29T11:06:37.033Z
Learning: In matrix testing scenarios where different versions of testing libraries are dynamically installed (like @testing-library/react-hooks vs @testing-library/react across React versions), using `any` types may be necessary due to unstable type dependencies, rather than importing specific types that may not be available in all test configurations.
src/useAsyncMemoSuspense.ts (1)
Learnt from: davemecha
PR: davemecha/use-async-effekt#3
File: src/__tests__/test-utils.ts:6-8
Timestamp: 2025-06-29T11:06:37.033Z
Learning: In matrix testing scenarios where different versions of testing libraries are dynamically installed (like @testing-library/react-hooks vs @testing-library/react across React versions), using `any` types may be necessary due to unstable type dependencies, rather than importing specific types that may not be available in all test configurations.
src/__tests__/useAsyncMemoSuspense.test.tsx (1)
Learnt from: davemecha
PR: davemecha/use-async-effekt#3
File: src/__tests__/test-utils.ts:6-8
Timestamp: 2025-06-29T11:06:37.033Z
Learning: In matrix testing scenarios where different versions of testing libraries are dynamically installed (like @testing-library/react-hooks vs @testing-library/react across React versions), using `any` types may be necessary due to unstable type dependencies, rather than importing specific types that may not be available in all test configurations.
🔇 Additional comments (20)
src/index.ts (1)
3-3: LGTM! Clean export addition.The new export for
useAsyncMemoSuspensefollows the established pattern and properly exposes the hook for external use.src/__tests__/useAsyncMemo.test.ts (1)
7-7: LGTM! Good testing practice.Adding
jest.clearAllMocks()to the beforeEach block ensures proper test isolation by resetting all mock state between tests, preventing potential test interference.package.json (3)
5-5: LGTM! Good addition for tree-shaking optimization.Adding
"sideEffects": falsecorrectly indicates that this package is safe for tree-shaking, which will help bundlers eliminate unused code.
63-63: LGTM! Safe dependency upgrade.The ts-jest upgrade from ^29.1.0 to ^29.4.0 is a minor version bump that should provide bug fixes and improvements without breaking changes.
80-81: LGTM! Appropriate coverage exclusion.Excluding test-utils.ts files from coverage collection is correct since these are testing infrastructure files that don't need coverage tracking.
README.md (3)
5-16: LGTM! Improved badge accessibility.Wrapping the badges with anchor tags enhances accessibility and user experience by making the badges clickable links to relevant resources.
315-315: LGTM! Necessary ESLint configuration updates.Adding
useAsyncMemoSuspenseto the ESLint configuration ensures proper dependency checking for the new hook.Also applies to: 330-330
365-410: LGTM! Comprehensive documentation for the new hook.The documentation for
useAsyncMemoSuspenseis thorough and includes:
- Clear API reference with parameters and return types
- Important notes about SSR behavior and client component requirements
- Proper experimental status warning
- Practical usage example with Suspense
The SSR behavior explanation is particularly valuable for developers using Next.js and similar frameworks.
src/useAsyncMemoSuspense.ts (5)
1-8: Well-structured type definitionsThe discriminated union pattern for
CacheEntryprovides excellent type safety and clear state management.
9-10: Correct dependency comparison implementationUsing
Object.ismatches React's built-in dependency comparison behavior and correctly handles edge cases like NaN and -0.
30-78: Excellent documentationThe JSDoc is comprehensive, clearly marks the hook as experimental, and provides a practical example. The SSR behavior explanation is particularly helpful.
84-89: Potential hydration mismatchReturning
undefinedon the server while potentially returning a value on the client could cause hydration mismatches. Ensure consuming components handle this appropriately, perhaps by always showing the Suspense fallback initially.Do you want me to verify if this pattern could cause hydration issues in Next.js or other SSR frameworks?
90-117: Solid implementation of Suspense integrationThe cache management and promise handling logic is well-implemented:
- Correctly handles both sync and async factories
- Properly maintains cache entry references
- Follows React Suspense patterns by throwing promises
src/__tests__/useAsyncMemoSuspense.test.tsx (7)
1-84: Well-structured test setupExcellent test infrastructure with typed helper components, realistic usage examples, and proper error boundary implementation. Good application of the learning about test-utils for matrix testing.
85-160: Thorough testing of basic functionalityGood coverage of both async suspension and sync value handling. Proper use of fake timers and React Testing Library patterns.
162-211: Comprehensive error handling testsBoth synchronous and asynchronous error scenarios are well-tested with proper error boundary integration.
213-296: Excellent dependency and scoping testsThorough verification of dependency tracking and cache isolation through scoping. The factory call count assertions effectively verify caching behavior.
298-365: Great testing of React 18 concurrent featuresExcellent test demonstrating proper Suspense behavior with
startTransition, correctly verifying that stale content is shown during transitions.
622-658: Function caching test confirms the cache key issueThis test effectively demonstrates the cache key collision problem mentioned in the implementation review. When different functions are used, they should create separate cache entries, which works here only because the deps are different.
367-706: Outstanding edge case coverageExceptional test coverage including:
- Cache sharing between components
- Complex object dependencies
- IEEE 754 zero handling (-0 vs 0)
- Performance under rapid changes
- Empty and undefined dependency arrays
The test suite thoroughly exercises the implementation.
Summary by CodeRabbit
New Features
useAsyncMemoSuspense, for memoizing asynchronous computations with React Suspense support.useAsyncMemoSuspensein the README.Tests
useAsyncMemoSuspensecovering async/sync behavior, caching, dependency management, error handling, and concurrent features.useAsyncMemoby ensuring mocks are reset before each test.Documentation
Chores
ts-jestdependency for testing.Refactor