Repository navigation
refactor(mosaic): read the current time through useNow instead of during render - #10098
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 743602d The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: This review used your included allowance. 4 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughAdds the Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to API key expiration is calculated when the key is submitted. The brief possibility of a stale preview near midnight does not warrant delaying the merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-biometrics
@clerk/expo-google-signin
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/mosaic
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
| export function useNow({ updateInterval }: { updateInterval?: number } = {}): Date { | ||
| const [now, setNow] = useState(() => new Date()); | ||
|
|
||
| useEffect(() => { | ||
| if (updateInterval === undefined) { | ||
| return; | ||
| } | ||
| const id = window.setInterval(() => setNow(new Date()), updateInterval); | ||
| return () => window.clearInterval(id); | ||
| }, [updateInterval]); | ||
|
|
||
| return now; | ||
| } |
There was a problem hiding this comment.
This wont work well with SSR, the state initializer will have different times on the server and the client, causing hydration mismatches.
I suggest we add the new Date() to the MosaicProvider instead and put it on a context that the useNow hook reads its initial state from. The interval still belongs here since different components might want different granularity for their reactivity (re-rendering all of these every second is heavy).
When we have that shape from the start, it's a lot easier to add SSR support by passing the new Date() result from server->client. That part would live in our framework SDKs though, they all do it a bit differently (but all already pass data server->client).
An optional improvement could be to have all the intervals at the top as well so they de-duplicate. All components that should update every minute do so together instead of on their own timer, and we end up with less timers. I think this leads to quite a bit more complexity though so I'd defer that for later/if we notice it's necessary.
There was a problem hiding this comment.
haven't been considering ssr support to much since their client components, but a good reminder to keep in mind 👍🏼 673b8a6
| export function useNow({ updateInterval }: { updateInterval?: number } = {}): Date { | ||
| const [now, setNow] = useState(() => new Date()); | ||
|
|
||
| useEffect(() => { | ||
| if (updateInterval === undefined) { | ||
| return; | ||
| } | ||
| const id = window.setInterval(() => setNow(new Date()), updateInterval); | ||
| return () => window.clearInterval(id); | ||
| }, [updateInterval]); | ||
|
|
||
| return now; | ||
| } |
Description
Adds a
useNow({ updateInterval })hook to Mosaic so components stop reading the clock during render, which breaks React's purity rule. Raised in #9983 (comment).The hook mirrors next-intl's
useNow: it reads the time once in a lazyuseStateinitializer and, whenupdateIntervalis set, refreshes it from asetIntervalin an effect.Applied to the two existing render-time reads:
lastUsedAtrelative labels (now also refresh every minute).Reverification's resend countdown already uses this pattern by hand and is left as is, since it derives whether to tick from
nowitself.#9983 can rebase on this and pass
relativeTo: useNow({ updateInterval: 60_000 })toformatRelative.Checklist
pnpm testruns as expected. (@clerk/mosaic)pnpm buildruns as expected. (@clerk/mosaic)Type of change