Revert "feat(shared): Remove SWR (#7455)" #7565
Conversation
This reverts commit 78430f8.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
📝 WalkthroughWalkthroughThis PR implements a dual-implementation strategy for data-fetching hooks, enabling runtime or compile-time selection between SWR and React Query implementations. It introduces a Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @packages/shared/src/react/billing/useStripeClerkLibs.swr.tsx:
- Around line 20-37: The SWR hook useStripeClerkLibs currently always invokes
the Stripe loader; add the same billing gate used in the React Query version by
importing and calling useBillingHookEnabled() and only enable the useSWR fetch
when billingEnabled is true (e.g., use a null key or conditional fetch function)
so that clerk.__internal_loadStripeJs() is not called when billing is disabled;
update useStripeClerkLibs to return null when billing is disabled and keep
existing SWR options (keepPreviousData, revalidateOnFocus, dedupingInterval)
when enabled.
In @packages/shared/src/react/hooks/__tests__/usePlans.spec.tsx:
- Around line 264-277: The test captures the `calls` array before the
asynchronous `waitFor`, causing a race where `calls` may be empty and make
assertions pass vacuously; move the extraction of `calls` (currently using
`getPlansSpy.mock.calls.map(...)`) to after the appropriate `waitFor` completes
(inside each branch that awaits `waitFor`) so the assertions on `calls` reflect
the actual recorded `getPlansSpy` invocations when `isRQ` (via
globalThis.__CLERK_USE_RQ__) is true or false.
In @packages/shared/src/react/hooks/usePagesOrInfinite.rq.tsx:
- Around line 203-208: The pagination checks multiply offsetCount by
pageSizeRef.current again, causing wrong results when initialPage > 1; update
the non-infinite branches in hasNextPage and hasPreviousPage to compare against
offsetCount (not offsetCount * pageSizeRef.current): for hasNextPage use count -
offsetCount > page * pageSizeRef.current, and for hasPreviousPage use (page - 1)
* pageSizeRef.current > offsetCount. Keep the triggerInfinite branches using
infiniteQuery as-is.
🧹 Nitpick comments (1)
packages/shared/src/react/billing/useInitializePaymentMethod.swr.tsx (1)
44-52: Silent error suppression during auto-initialization.The empty catch block silently swallows errors during the automatic initialization triggered by
useEffect. While this may be intentional to prevent unhandled promise rejections on mount, it could make debugging difficult if initialization consistently fails.Consider logging the error at a debug level or ensuring the error is surfaced through
useSWRMutation's error state for consumers to handle.Optional: Log errors for debugging
useEffect(() => { if (!resource?.id) { return; } trigger().catch(() => { - // ignore errors + // Errors are available via the hook's error state; + // this catch prevents unhandled promise rejection warnings }); }, [resource?.id, trigger]);
| function useStripeClerkLibs(): UseStripeClerkLibsResult { | ||
| const clerk = useClerk(); | ||
|
|
||
| const swr = useSWR( | ||
| 'clerk-stripe-sdk', | ||
| async () => { | ||
| const loadStripe = (await clerk.__internal_loadStripeJs()) as LoadStripeFn; | ||
| return { loadStripe }; | ||
| }, | ||
| { | ||
| keepPreviousData: true, | ||
| revalidateOnFocus: false, | ||
| dedupingInterval: Infinity, | ||
| }, | ||
| ); | ||
|
|
||
| return swr.data ?? null; | ||
| } |
There was a problem hiding this comment.
Behavioral inconsistency: Missing billingEnabled guard.
The React Query implementation (useStripeClerkLibs.rq.tsx) uses useBillingHookEnabled() to conditionally enable the query, but this SWR implementation fetches unconditionally. This means:
- RQ: Only loads Stripe SDK when billing is enabled
- SWR: Always attempts to load Stripe SDK
This behavioral difference could cause unexpected network requests or errors when billing is disabled.
Proposed fix to align behavior
import type { loadStripe } from '@stripe/stripe-js';
import { useSWR } from '../clerk-swr';
import { useClerk } from '../hooks/useClerk';
+import { useBillingHookEnabled } from './useBillingHookEnabled';
// ... types ...
function useStripeClerkLibs(): UseStripeClerkLibsResult {
const clerk = useClerk();
+ const billingEnabled = useBillingHookEnabled();
const swr = useSWR(
- 'clerk-stripe-sdk',
+ billingEnabled ? 'clerk-stripe-sdk' : null,
async () => {
const loadStripe = (await clerk.__internal_loadStripeJs()) as LoadStripeFn;
return { loadStripe };
},
{
keepPreviousData: true,
revalidateOnFocus: false,
dedupingInterval: Infinity,
},
);
return swr.data ?? null;
}🤖 Prompt for AI Agents
In @packages/shared/src/react/billing/useStripeClerkLibs.swr.tsx around lines 20
- 37, The SWR hook useStripeClerkLibs currently always invokes the Stripe
loader; add the same billing gate used in the React Query version by importing
and calling useBillingHookEnabled() and only enable the useSWR fetch when
billingEnabled is true (e.g., use a null key or conditional fetch function) so
that clerk.__internal_loadStripeJs() is not called when billing is disabled;
update useStripeClerkLibs to return null when billing is disabled and keep
existing SWR options (keepPreviousData, revalidateOnFocus, dedupingInterval)
when enabled.
| const isRQ = Boolean((globalThis as any).__CLERK_USE_RQ__); | ||
| const calls = getPlansSpy.mock.calls.map(call => call[0]?.for); | ||
| expect(calls.every(value => value === 'user')).toBe(true); | ||
|
|
||
| if (isRQ) { | ||
| await waitFor(() => expect(getPlansSpy.mock.calls.length).toBeGreaterThanOrEqual(1)); | ||
| expect(calls.every(value => value === 'user')).toBe(true); | ||
| } else { | ||
| await waitFor(() => expect(getPlansSpy.mock.calls.length).toBe(1)); | ||
| expect(getPlansSpy.mock.calls[0][0]).toEqual( | ||
| expect.objectContaining({ | ||
| for: 'user', | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Potential race condition: calls captured before waitFor completes.
The calls array is populated on line 265 before the waitFor on line 268 executes. In the RQ branch, if getPlansSpy.mock.calls is still empty when line 265 runs, the calls.every(...) check on line 269 will return true vacuously (empty array), potentially masking test failures.
Consider moving the calls extraction inside or after the waitFor:
Suggested fix
if (isRQ) {
await waitFor(() => expect(getPlansSpy.mock.calls.length).toBeGreaterThanOrEqual(1));
+ const calls = getPlansSpy.mock.calls.map(call => call[0]?.for);
expect(calls.every(value => value === 'user')).toBe(true);
} else {
await waitFor(() => expect(getPlansSpy.mock.calls.length).toBe(1));
expect(getPlansSpy.mock.calls[0][0]).toEqual(
expect.objectContaining({
for: 'user',
}),
);
}And move the const calls declaration inside the if block or after both branches complete their respective waitFor.
🤖 Prompt for AI Agents
In @packages/shared/src/react/hooks/__tests__/usePlans.spec.tsx around lines 264
- 277, The test captures the `calls` array before the asynchronous `waitFor`,
causing a race where `calls` may be empty and make assertions pass vacuously;
move the extraction of `calls` (currently using
`getPlansSpy.mock.calls.map(...)`) to after the appropriate `waitFor` completes
(inside each branch that awaits `waitFor`) so the assertions on `calls` reflect
the actual recorded `getPlansSpy` invocations when `isRQ` (via
globalThis.__CLERK_USE_RQ__) is true or false.
| const hasNextPage = triggerInfinite | ||
| ? Boolean(infiniteQuery.hasNextPage) | ||
| : count - offsetCount * pageSizeRef.current > page * pageSizeRef.current; | ||
| const hasPreviousPage = triggerInfinite | ||
| ? Boolean(infiniteQuery.hasPreviousPage) | ||
| : (page - 1) * pageSizeRef.current > offsetCount * pageSizeRef.current; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -i "usepages\|infinite"Repository: clerk/javascript
Length of output: 693
🏁 Script executed:
cat -n packages/shared/src/react/hooks/usePagesOrInfinite.rq.tsx | head -250Repository: clerk/javascript
Length of output: 11324
🏁 Script executed:
# Find tests for this hook
find . -type f \( -name "*.test.*" -o -name "*.spec.*" \) | xargs grep -l "usePagesOrInfinite" 2>/dev/nullRepository: clerk/javascript
Length of output: 276
🏁 Script executed:
# Search for any tests related to pagination in the shared package
find packages/shared -type f \( -name "*.test.*" -o -name "*.spec.*" \) | head -20Repository: clerk/javascript
Length of output: 1082
🏁 Script executed:
cat -n packages/shared/src/react/hooks/__tests__/usePagesOrInfinite.spec.tsRepository: clerk/javascript
Length of output: 41585
Fix pagination logic for hasNextPage and hasPreviousPage when initialPage > 1.
The calculations on lines 205 and 208 multiply offsetCount by pageSizeRef.current again, but offsetCount is already computed as (initialPageRef.current - 1) * pageSizeRef.current on line 201. This causes incorrect pagination state detection.
Compare with line 202 where pageCount correctly uses (count - offsetCount) without extra multiplication.
Suggested fix
const hasNextPage = triggerInfinite
? Boolean(infiniteQuery.hasNextPage)
- : count - offsetCount * pageSizeRef.current > page * pageSizeRef.current;
+ : count - offsetCount > page * pageSizeRef.current;
const hasPreviousPage = triggerInfinite
? Boolean(infiniteQuery.hasPreviousPage)
- : (page - 1) * pageSizeRef.current > offsetCount * pageSizeRef.current;
+ : (page - 1) * pageSizeRef.current > offsetCount;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const hasNextPage = triggerInfinite | |
| ? Boolean(infiniteQuery.hasNextPage) | |
| : count - offsetCount * pageSizeRef.current > page * pageSizeRef.current; | |
| const hasPreviousPage = triggerInfinite | |
| ? Boolean(infiniteQuery.hasPreviousPage) | |
| : (page - 1) * pageSizeRef.current > offsetCount * pageSizeRef.current; | |
| const hasNextPage = triggerInfinite | |
| ? Boolean(infiniteQuery.hasNextPage) | |
| : count - offsetCount > page * pageSizeRef.current; | |
| const hasPreviousPage = triggerInfinite | |
| ? Boolean(infiniteQuery.hasPreviousPage) | |
| : (page - 1) * pageSizeRef.current > offsetCount; |
🤖 Prompt for AI Agents
In @packages/shared/src/react/hooks/usePagesOrInfinite.rq.tsx around lines 203 -
208, The pagination checks multiply offsetCount by pageSizeRef.current again,
causing wrong results when initialPage > 1; update the non-infinite branches in
hasNextPage and hasPreviousPage to compare against offsetCount (not offsetCount
* pageSizeRef.current): for hasNextPage use count - offsetCount > page *
pageSizeRef.current, and for hasPreviousPage use (page - 1) *
pageSizeRef.current > offsetCount. Keep the triggerInfinite branches using
infiniteQuery as-is.
This reverts commit 78430f8.
Description
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.