Skip to content

Conversation

@panteliselef
Copy link
Member

@panteliselef panteliselef commented Feb 7, 2025

Description

Pause session touch when browser is offline, and resume it when the device comes back online. When it is back online it will call touch once.

Checklist

  • pnpm test runs as expected.
  • pnpm build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@changeset-bot
Copy link

changeset-bot bot commented Feb 7, 2025

🦋 Changeset detected

Latest commit: 1366045

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@clerk/clerk-js Patch
@clerk/chrome-extension Patch
@clerk/clerk-expo Patch

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

@vercel
Copy link

vercel bot commented Feb 7, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
clerk-js-sandbox ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 17, 2025 5:52pm

@panteliselef panteliselef force-pushed the elef/sdki-605-dont-refresh-tokens-in-the-background-if-device-is-offline branch from 8847d45 to 9184b06 Compare February 7, 2025 13:31
@panteliselef panteliselef requested a review from anagstef February 7, 2025 14:06
@panteliselef panteliselef self-assigned this Feb 7, 2025
'@clerk/clerk-js': patch
---

Pause session touch when browser is offline, and resume it when the device comes back online.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you include the token polling also?

Copy link
Member

@octoper octoper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 Great work!

@panteliselef panteliselef force-pushed the elef/sdki-605-dont-refresh-tokens-in-the-background-if-device-is-offline branch from 5bb7abb to 0e8d7b9 Compare February 10, 2025 17:31
This reverts commit 40d0c80.
@panteliselef
Copy link
Member Author

This improvement uncovered a bug with our sign-out broadcasting solution, avoid merging until resolved.

// is updated as part of the scheduled microtask. Our existing event-based mechanism to update the cookie schedules a task, and so the cookie
// is updated too late and not guaranteed to be fresh before the refetch occurs.
void this.refreshSessionToken({ updateCookieImmediately: true });
this.offlineScheduler.schedule(() => this.refreshSessionToken({ updateCookieImmediately: true }));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to make sure wrapping this call doesn't impact the behavior described in the comment above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While online, we are executing it instantly, so it does not affect the behaviour the comment describes. While offline, I think it still works, because the application requests will either fail, or will not fire and when they do fire the libraries will still use setTimeout(cb,0).

#options: ClerkOptions = {};
#pageLifecycle: ReturnType<typeof createPageLifecycle> | null = null;
#touchThrottledUntil = 0;
#offlineScheduler = createOfflineScheduler();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not immediately clear that this de-dupes all calls to .schedule() when offline, regardless of what function is provided. Should this be called sessionTouchOfflineScheduler maybe? I wouldn't want someone finding this and using it elsewhere if they aren't aware of this behavior.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, yeah we can change the name.

@panteliselef
Copy link
Member Author

Update on this:

This improvement uncovered a bug with our sign-out broadcasting solution, avoid merging until resolved.

We will be addressing the bug of broadcasting sign out on another PR. We are already investigating issues with our sign out mechanism and there is no reason to delay the release of this PR.

@panteliselef panteliselef enabled auto-merge (squash) February 17, 2025 17:53
@panteliselef panteliselef merged commit cab9408 into main Feb 17, 2025
29 checks passed
@panteliselef panteliselef deleted the elef/sdki-605-dont-refresh-tokens-in-the-background-if-device-is-offline branch February 17, 2025 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants