Uh oh!
There was an error while loading. Please reload this page.
refactor(js): extract the refresh scheduler from TokenCache - #9042
refactor(js): extract the refresh scheduler from TokenCache#9042jacekradko wants to merge 3 commits into
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 55e8671 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a refresh scheduler for per-key token expiration and proactive refresh timing, wires ChangesRefreshScheduler abstraction and token cache integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
@clerk/astro@clerk/backend@clerk/chrome-extension@clerk/clerk-js@clerk/electron@clerk/electron-passkeys@clerk/eslint-plugin@clerk/expo@clerk/expo-passkeys@clerk/express@clerk/fastify@clerk/hono@clerk/localizations@clerk/nextjs@clerk/nuxt@clerk/react@clerk/react-router@clerk/shared@clerk/tanstack-react-start@clerk/testing@clerk/ui@clerk/upgrade@clerk/vuecommit: |
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/clerk-js/src/core/refreshScheduler.ts (1)
65-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
anyin theunref()path.This helper can stay cross-runtime without dropping type safety; a small guard is enough to call
unref()only on Node timer handles.♻️ Proposed refactor
+interface UnrefTimer {+ unref(): void;+}++const hasUnref = (+ value: ReturnType<typeof setTimeout>,+): value is ReturnType<typeof setTimeout> & UnrefTimer =>+ typeof value === 'object' && value !== null && 'unref' in value && typeof value.unref === 'function';+ const armTimer = (callback: () => void, delayMs: number): ReturnType<typeof setTimeout> => { const id = setTimeout(callback, delayMs); - if (typeof (id as any).unref === 'function') {- (id as any).unref();+ if (hasUnref(id)) {+ id.unref(); } return id; };As per coding guidelines, "Avoid
anytype - preferunknownwhen type is uncertain, then narrow with type guards" and "Noanytypes without justification in code review".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/clerk-js/src/core/refreshScheduler.ts` around lines 65 - 69, The armTimer helper in refreshScheduler currently uses an any cast to call unref() on the timer handle, which violates the no-any guideline. Update armTimer to use a type-safe narrowing approach (for example, a small guard on the setTimeout result) so unref() is only called when the handle actually supports it, while keeping the cross-runtime behavior intact.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/clerk-js/src/core/refreshScheduler.ts`:
- Around line 65-69: The armTimer helper in refreshScheduler currently uses an
any cast to call unref() on the timer handle, which violates the no-any
guideline. Update armTimer to use a type-safe narrowing approach (for example, a
small guard on the setTimeout result) so unref() is only called when the handle
actually supports it, while keeping the cross-runtime behavior intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: bf4cce79-ae41-43b1-9ece-833e22a0cc19
📒 Files selected for processing (5)
.changeset/sdk-140-refresh-scheduler.mdpackages/clerk-js/src/core/__tests__/refreshScheduler.test.tspackages/clerk-js/src/core/__tests__/tokenCache.test.tspackages/clerk-js/src/core/refreshScheduler.tspackages/clerk-js/src/core/tokenCache.ts
Reconciles the tokenCache.ts conflict with #9030 (apply session tokens monotonically), which landed after this branch diverged and rewrote the setInternal resolver this branch extracts. Keeps #9030's monotonic winner selection (baseline chaining, pickFreshestJwt, dropIfCurrent, already-expired drop) intact and delegates all timer arming to the extracted refreshScheduler.
Hello 👋 We currently close PRs after 60 days of inactivity. It's been 50 days since the last update here. If we missed this PR, please reply here. Otherwise, we'll close this PR in 10 days. Thanks for being a part of the Clerk community! 🙏 |
Step 4 of the TokenCache decouple (SDK-117): lifts both
setTimeouttimers (expiration cleanup + proactive refresh) out oftokenCache.tsinto acreateRefreshSchedulercollaborator with an injected clock, matching thetokenStore/keyResolvershape from the earlier steps. Timers staysetTimeout-based and the clock only suppliesnow(), so the existing ~30 fake-timer tests pass unchanged.Two things worth a close read:
Refresh delays are now recomputed from the token's absolute
exp(exp - now() - leeway - lead) rather than a fixedexpiresInmeasured from cache-set time. Identical for freshly fetched tokens (iat == now), but for a token hydrated from the session cookie before the tab loaded, today's code schedules the proactive refresh a full lifetime out (past expiry, so it never fires proactively); the recompute fixes that.refreshScheduler.test.tspins it down (a past-issuance token fires at 13s, not 43s).deleteKeyis both the expiration-timer callback and the resolver chain's.catch, so itsscheduler.cancel(key)stays inside thestore.get(key) === valueidentity guard. Otherwise a stale rejected resolver could disarm the replacement entry's live timers. A new tokenCache test covers that path.Deliberately deferred to a follow-up: the startup catch-up sweep, re-arm on visibility/online (overlaps
AuthCookieService's existing focus/visibility refresh), and failure backoff.Closes SDK-140.
Summary by CodeRabbit
New Features
Bug Fixes