feat(shared): Replace callWithRetry and runWithExponentialBackoff with retry - #5144

Merged
nikosdouvlis merged 4 commits into
mainfrom
nikos/retry-errors-in-clerkjs
Feb 25, 2025
Merged

feat(shared): Replace callWithRetry and runWithExponentialBackoff with retry#5144
nikosdouvlis merged 4 commits into
mainfrom
nikos/retry-errors-in-clerkjs

Conversation

@nikosdouvlis

@nikosdouvlisnikosdouvlis commented Feb 13, 2025•
edited
Loading

Copy link
Copy Markdown
Member

Description

  • Replace callWithRetry with runWithExponentialBackOff so we only have a single retry mechanism in use
  • Rename runWithExponentialBackOff to retry for clarity
  • Add the following config options to runWithExponentialBackOff :
    • retryImmediately (boolean): Controls whether the helper should retry the operation immediately. If true, the exponential backoff delay will start only once this retry is over. Defaults to true.
    • jitter (boolean): If true, the intervals will be multiplied by a factor of [1,2]. Defaults to true.
  • Increase unit test coverage (retry.test.ts)

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

changeset-botBot commented Feb 13, 2025•
edited
Loading

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: c48f5d3

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

This PR includes changesets to release 20 packages
NameType
@clerk/sharedMajor
@clerk/agent-toolkitPatch
@clerk/astroPatch
@clerk/backendPatch
@clerk/chrome-extensionPatch
@clerk/clerk-jsPatch
@clerk/elementsPatch
@clerk/expo-passkeysPatch
@clerk/clerk-expoPatch
@clerk/expressPatch
@clerk/fastifyPatch
@clerk/nextjsPatch
@clerk/nuxtPatch
@clerk/react-routerPatch
@clerk/clerk-reactPatch
@clerk/remixPatch
@clerk/tanstack-startPatch
@clerk/testingPatch
@clerk/uiPatch
@clerk/vuePatch

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

vercelBot commented Feb 13, 2025•
edited
Loading

Copy link
Copy Markdown

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

NameStatusPreviewCommentsUpdated (UTC)
clerk-js-sandbox🛑 Canceled (Inspect)Feb 14, 2025 0:51am

@nikosdouvlis
nikosdouvlisforce-pushed the nikos/retry-errors-in-clerkjs branch from 110a8c3 to f0aa6edCompareFebruary 13, 2025 11:21
@nikosdouvlis

nikosdouvlis commented Feb 13, 2025

Copy link
Copy Markdown
MemberAuthor

!allow-major

panteliselef
panteliselef reviewed Feb 13, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts Outdated
LauraBeatris
LauraBeatris reviewed Feb 13, 2025
View reviewed changes
Comment threadpackages/shared/src/__tests__/retry.test.ts Outdated
anagstef
anagstef reviewed Feb 13, 2025
View reviewed changes
Comment thread.changeset/metal-bobcats-wash.md Outdated
LekoArts
LekoArts suggested changes Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts Outdated
Comment threadpackages/shared/src/retry.ts Outdated
Comment threadpackages/shared/src/retry.ts
Comment threadpackages/shared/src/retry.ts Outdated
Comment on lines +32 to +37
retryImmediatelyOnce: boolean;
/**
* The delay for the immediate retry.
* @default 100
*/
retryImmediatelyDelay: Milliseconds;

@LekoArtsLekoArtsFeb 14, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These two options feel weird, I can't put a finger on it but this seems like something that should be only one option and be renamed.

  • Retrying immediately already implies that it can only happen once, right? Afterwards it's not immediate anymore
  • Technically "immediate" would be a delay of 0 so the current default of 100 is already quite arbitrary. Why should we need to change this from a case by case basis?

So I'd just put those two options into one called retryImmediate: boolean

@nikosdouvlisnikosdouvlisFeb 14, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Tagging @panteliselef as he was also interested about this - Im also open to suggestions to improve this further.
There are at least two cases where we might want to immediately retry, before applying the backoff strategy:

  • mission-critical operations that need to be completed as fast as possible
  • transient errors on the network level (think about mobile devices)

This property decouples the immediate retry with the rest of the backoff logic, as its very hard to tune the options using a combination of factor + initialDelay otherwise.

@nikosdouvlisnikosdouvlisFeb 14, 2025•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I'm going to merge these two into a single retryImmediately: boolean with a default delay of 100ms. If we ever need to change this, we can add a new prop then

@nikosdouvlisnikosdouvlisFeb 14, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done

Comment threadpackages/shared/src/retry.ts
@nikosdouvlis
nikosdouvlisforce-pushed the nikos/retry-errors-in-clerkjs branch from f0aa6ed to 6dc4e5dCompareFebruary 14, 2025 11:22
@nikosdouvlis
nikosdouvlisforce-pushed the nikos/retry-errors-in-clerkjs branch from 6dc4e5d to b0646a7CompareFebruary 14, 2025 11:30
@nikosdouvlis
nikosdouvlisforce-pushed the nikos/retry-errors-in-clerkjs branch from b0646a7 to b1f5bc5CompareFebruary 14, 2025 11:42
panteliselef
panteliselef approved these changes Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
LekoArts
LekoArts reviewed Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/__tests__/retry.test.ts Outdated
Comment threadpackages/shared/src/__tests__/retry.test.ts Outdated
Comment threadpackages/shared/src/__tests__/retry.test.ts Outdated
nikosdouvlisand others added 3 commits February 14, 2025 14:47
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
3f3284a
Co-authored-by: Lennart <lekoarts@gmail.com>
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
6e9fb24
Co-authored-by: Lennart <lekoarts@gmail.com>
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
c48f5d3
Co-authored-by: Lennart <lekoarts@gmail.com>
brkalow
brkalow reviewed Feb 18, 2025
View reviewed changes
Comment threadpackages/backend/src/tokens/keys.ts
}
const fetcher = () => fetchJWKSFromBAPI(apiUrl, secretKey, apiVersion);
const { keys } = await callWithRetry<{ keys: JsonWebKeyWithKid[] }>(fetcher);
const fetcher = () => fetchJWKSFromBAPI(apiUrl, secretKey, apiVersion) as Promise<{ keys: JsonWebKeyWithKid[] }>;

@brkalowbrkalowFeb 18, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

❓ why do we need to cast here?

brkalow
brkalow approved these changes Feb 18, 2025
View reviewed changes

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👏

@nikosdouvlis
nikosdouvlis merged commit 7ae77b7 into mainFeb 25, 2025
@nikosdouvlis
nikosdouvlis deleted the nikos/retry-errors-in-clerkjs branch February 25, 2025 09:14
@clerk-cookieclerk-cookie mentioned this pull request Feb 25, 2025
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brkalowbrkalowbrkalow approved these changes
+4 more reviewers
@LauraBeatrisLauraBeatrisLauraBeatris left review comments
@anagstefanagstefanagstef approved these changes
@LekoArtsLekoArtsLekoArts approved these changes
@panteliselefpanteliselefpanteliselef approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backendclerk-js

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@nikosdouvlis@brkalow@anagstef@LekoArts@panteliselef@LauraBeatris@clerk-cookie
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
feat(shared): Replace callWithRetry and runWithExponentialBackoff with retry - #5144
Merged
nikosdouvlis merged 4 commits into
mainfrom 
nikos/retry-errors-in-clerkjs
Feb 25, 2025
Merged
feat(shared): Replace callWithRetry and runWithExponentialBackoff with retry#5144
nikosdouvlis merged 4 commits into
mainfrom 
nikos/retry-errors-in-clerkjs
Conversation
@nikosdouvlis
@nikosdouvlisnikosdouvlis
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Description
  • Replace callWithRetry with runWithExponentialBackOff so we only have a single retry mechanism in use
  • Rename runWithExponentialBackOff to retry for clarity
  • Add the following config options to runWithExponentialBackOff :

    • retryImmediately (boolean): Controls whether the helper should retry the operation immediately. If true, the exponential backoff delay will start only once this retry is over. Defaults to true.
    • jitter (boolean): If true, the intervals will be multiplied by a factor of [1,2]. Defaults to true.
  • Increase unit test coverage (retry.test.ts)
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
changeset-botBot
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

🦋 Changeset detected
Latest commit: c48f5d3
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 20 packages
NameType
@clerk/sharedMajor
@clerk/agent-toolkitPatch
@clerk/astroPatch
@clerk/backendPatch
@clerk/chrome-extensionPatch
@clerk/clerk-jsPatch
@clerk/elementsPatch
@clerk/expo-passkeysPatch
@clerk/clerk-expoPatch
@clerk/expressPatch
@clerk/fastifyPatch
@clerk/nextjsPatch
@clerk/nuxtPatch
@clerk/react-routerPatch
@clerk/clerk-reactPatch
@clerk/remixPatch
@clerk/tanstack-startPatch
@clerk/testingPatch
@clerk/uiPatch
@clerk/vuePatch
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
vercelBot
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎
NameStatusPreviewCommentsUpdated (UTC)
clerk-js-sandbox🛑 Canceled (Inspect)Feb 14, 2025 0:51am
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
110a8c3 to
f0aa6edCompareFebruary 13, 2025 11:21
@nikosdouvlis
nikosdouvlis
commented
Feb 13, 2025

Copy link


Copy Markdown

MemberAuthor
!allow-major
panteliselef
panteliselef
reviewed
Feb 13, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
Outdated

LauraBeatris
LauraBeatris
reviewed
Feb 13, 2025
View reviewed changes
Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

anagstef
anagstef
reviewed
Feb 13, 2025
View reviewed changes
Comment thread.changeset/metal-bobcats-wash.md
Outdated

LekoArts
LekoArts
suggested changes
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
Outdated

Comment threadpackages/shared/src/retry.ts
Outdated

Comment threadpackages/shared/src/retry.ts
Comment threadpackages/shared/src/retry.ts
Outdated


Comment on lines

+32
 to 
+37

 retryImmediatelyOnce: boolean;
 /**
 * The delay for the immediate retry.
 * @default 100
 */
 retryImmediatelyDelay: Milliseconds;
@LekoArtsLekoArtsFeb 14, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

These two options feel weird, I can't put a finger on it but this seems like something that should be only one option and be renamed.
  • Retrying immediately already implies that it can only happen once, right? Afterwards it's not immediate anymore
  • Technically "immediate" would be a delay of 0 so the current default of 100 is already quite arbitrary. Why should we need to change this from a case by case basis?
So I'd just put those two options into one called retryImmediate: boolean
@nikosdouvlisnikosdouvlisFeb 14, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Tagging @panteliselef as he was also interested about this - Im also open to suggestions to improve this further.

There are at least two cases where we might want to immediately retry, before applying the backoff strategy:
  • mission-critical operations that need to be completed as fast as possible
  • transient errors on the network level (think about mobile devices)
This property decouples the immediate retry with the rest of the backoff logic, as its very hard to tune the options using a combination of factor + initialDelay otherwise.
@nikosdouvlisnikosdouvlisFeb 14, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I'm going to merge these two into a single retryImmediately: boolean with a default delay of 100ms. If we ever need to change this, we can add a new prop then
@nikosdouvlisnikosdouvlisFeb 14, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done
Comment threadpackages/shared/src/retry.ts
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
f0aa6ed to
6dc4e5dCompareFebruary 14, 2025 11:22
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
6dc4e5d to
b0646a7CompareFebruary 14, 2025 11:30
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
b0646a7 to
b1f5bc5CompareFebruary 14, 2025 11:42
panteliselef
panteliselef
approved these changes
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
LekoArts
LekoArts
reviewed
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

nikosdouvlisand others
added 3 commits
February 14, 2025 14:47
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
3f3284a
Co-authored-by: Lennart <lekoarts@gmail.com>
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
6e9fb24
Co-authored-by: Lennart <lekoarts@gmail.com>
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
c48f5d3
Co-authored-by: Lennart <lekoarts@gmail.com>
brkalow
brkalow
reviewed
Feb 18, 2025
View reviewed changes
Comment threadpackages/backend/src/tokens/keys.ts
 }
 const fetcher = () => fetchJWKSFromBAPI(apiUrl, secretKey, apiVersion);
 const { keys } = await callWithRetry<{ keys: JsonWebKeyWithKid[] }>(fetcher);
 const fetcher = () => fetchJWKSFromBAPI(apiUrl, secretKey, apiVersion) as Promise<{ keys: JsonWebKeyWithKid[] }>;
@brkalowbrkalowFeb 18, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

❓ why do we need to cast here?
brkalow
brkalow
approved these changes
Feb 18, 2025
View reviewed changes
@brkalowbrkalow
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

👏
@nikosdouvlis
nikosdouvlis
merged commit 7ae77b7
into
mainFeb 25, 2025
@nikosdouvlis
nikosdouvlis
deleted the

nikos/retry-errors-in-clerkjs

branch
February 25, 2025 09:14
@clerk-cookieclerk-cookie
mentioned this pull request
Feb 25, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@brkalowbrkalowbrkalow approved these changes

+4 more reviewers

@LauraBeatrisLauraBeatrisLauraBeatris left review comments
@anagstefanagstefanagstef approved these changes
@LekoArtsLekoArtsLekoArts approved these changes
@panteliselefpanteliselefpanteliselef approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

backendclerk-js

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@nikosdouvlis@brkalow@anagstef@LekoArts@panteliselef@LauraBeatris@clerk-cookie

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(shared): Replace callWithRetry and runWithExponentialBackoff with retry - #5144
Merged
nikosdouvlis merged 4 commits into
mainfrom 
nikos/retry-errors-in-clerkjs
Feb 25, 2025
Merged
feat(shared): Replace callWithRetry and runWithExponentialBackoff with retry#5144
nikosdouvlis merged 4 commits into
mainfrom 
nikos/retry-errors-in-clerkjs
Conversation
@nikosdouvlis
@nikosdouvlisnikosdouvlis
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Description
  • Replace callWithRetry with runWithExponentialBackOff so we only have a single retry mechanism in use
  • Rename runWithExponentialBackOff to retry for clarity
  • Add the following config options to runWithExponentialBackOff :

    • retryImmediately (boolean): Controls whether the helper should retry the operation immediately. If true, the exponential backoff delay will start only once this retry is over. Defaults to true.
    • jitter (boolean): If true, the intervals will be multiplied by a factor of [1,2]. Defaults to true.
  • Increase unit test coverage (retry.test.ts)
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
changeset-botBot
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

🦋 Changeset detected
Latest commit: c48f5d3
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 20 packages
NameType
@clerk/sharedMajor
@clerk/agent-toolkitPatch
@clerk/astroPatch
@clerk/backendPatch
@clerk/chrome-extensionPatch
@clerk/clerk-jsPatch
@clerk/elementsPatch
@clerk/expo-passkeysPatch
@clerk/clerk-expoPatch
@clerk/expressPatch
@clerk/fastifyPatch
@clerk/nextjsPatch
@clerk/nuxtPatch
@clerk/react-routerPatch
@clerk/clerk-reactPatch
@clerk/remixPatch
@clerk/tanstack-startPatch
@clerk/testingPatch
@clerk/uiPatch
@clerk/vuePatch
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
vercelBot
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎
NameStatusPreviewCommentsUpdated (UTC)
clerk-js-sandbox🛑 Canceled (Inspect)Feb 14, 2025 0:51am
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
110a8c3 to
f0aa6edCompareFebruary 13, 2025 11:21
@nikosdouvlis
nikosdouvlis
commented
Feb 13, 2025

Copy link


Copy Markdown

MemberAuthor
!allow-major
panteliselef
panteliselef
reviewed
Feb 13, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
Outdated

LauraBeatris
LauraBeatris
reviewed
Feb 13, 2025
View reviewed changes
Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

anagstef
anagstef
reviewed
Feb 13, 2025
View reviewed changes
Comment thread.changeset/metal-bobcats-wash.md
Outdated

LekoArts
LekoArts
suggested changes
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
Outdated

Comment threadpackages/shared/src/retry.ts
Outdated

Comment threadpackages/shared/src/retry.ts
Comment threadpackages/shared/src/retry.ts
Outdated


Comment on lines

+32
 to 
+37

 retryImmediatelyOnce: boolean;
 /**
 * The delay for the immediate retry.
 * @default 100
 */
 retryImmediatelyDelay: Milliseconds;
@LekoArtsLekoArtsFeb 14, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

These two options feel weird, I can't put a finger on it but this seems like something that should be only one option and be renamed.
  • Retrying immediately already implies that it can only happen once, right? Afterwards it's not immediate anymore
  • Technically "immediate" would be a delay of 0 so the current default of 100 is already quite arbitrary. Why should we need to change this from a case by case basis?
So I'd just put those two options into one called retryImmediate: boolean
@nikosdouvlisnikosdouvlisFeb 14, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Tagging @panteliselef as he was also interested about this - Im also open to suggestions to improve this further.

There are at least two cases where we might want to immediately retry, before applying the backoff strategy:
  • mission-critical operations that need to be completed as fast as possible
  • transient errors on the network level (think about mobile devices)
This property decouples the immediate retry with the rest of the backoff logic, as its very hard to tune the options using a combination of factor + initialDelay otherwise.
@nikosdouvlisnikosdouvlisFeb 14, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I'm going to merge these two into a single retryImmediately: boolean with a default delay of 100ms. If we ever need to change this, we can add a new prop then
@nikosdouvlisnikosdouvlisFeb 14, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done
Comment threadpackages/shared/src/retry.ts
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
f0aa6ed to
6dc4e5dCompareFebruary 14, 2025 11:22
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
6dc4e5d to
b0646a7CompareFebruary 14, 2025 11:30
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
b0646a7 to
b1f5bc5CompareFebruary 14, 2025 11:42
panteliselef
panteliselef
approved these changes
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
LekoArts
LekoArts
reviewed
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

nikosdouvlisand others
added 3 commits
February 14, 2025 14:47
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
3f3284a
Co-authored-by: Lennart <lekoarts@gmail.com>
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
6e9fb24
Co-authored-by: Lennart <lekoarts@gmail.com>
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
c48f5d3
Co-authored-by: Lennart <lekoarts@gmail.com>
brkalow
brkalow
reviewed
Feb 18, 2025
View reviewed changes
Comment threadpackages/backend/src/tokens/keys.ts
 }
 const fetcher = () => fetchJWKSFromBAPI(apiUrl, secretKey, apiVersion);
 const { keys } = await callWithRetry<{ keys: JsonWebKeyWithKid[] }>(fetcher);
 const fetcher = () => fetchJWKSFromBAPI(apiUrl, secretKey, apiVersion) as Promise<{ keys: JsonWebKeyWithKid[] }>;
@brkalowbrkalowFeb 18, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

❓ why do we need to cast here?
brkalow
brkalow
approved these changes
Feb 18, 2025
View reviewed changes
@brkalowbrkalow
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

👏
@nikosdouvlis
nikosdouvlis
merged commit 7ae77b7
into
mainFeb 25, 2025
@nikosdouvlis
nikosdouvlis
deleted the

nikos/retry-errors-in-clerkjs

branch
February 25, 2025 09:14
@clerk-cookieclerk-cookie
mentioned this pull request
Feb 25, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@brkalowbrkalowbrkalow approved these changes

+4 more reviewers

@LauraBeatrisLauraBeatrisLauraBeatris left review comments
@anagstefanagstefanagstef approved these changes
@LekoArtsLekoArtsLekoArts approved these changes
@panteliselefpanteliselefpanteliselef approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

backendclerk-js

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@nikosdouvlis@brkalow@anagstef@LekoArts@panteliselef@LauraBeatris@clerk-cookie

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(shared): Replace callWithRetry and runWithExponentialBackoff with retry - #5144
Merged
nikosdouvlis merged 4 commits into
mainfrom 
nikos/retry-errors-in-clerkjs
Feb 25, 2025
Merged
feat(shared): Replace callWithRetry and runWithExponentialBackoff with retry#5144
nikosdouvlis merged 4 commits into
mainfrom 
nikos/retry-errors-in-clerkjs
Conversation
@nikosdouvlis
@nikosdouvlisnikosdouvlis
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Description
  • Replace callWithRetry with runWithExponentialBackOff so we only have a single retry mechanism in use
  • Rename runWithExponentialBackOff to retry for clarity
  • Add the following config options to runWithExponentialBackOff :

    • retryImmediately (boolean): Controls whether the helper should retry the operation immediately. If true, the exponential backoff delay will start only once this retry is over. Defaults to true.
    • jitter (boolean): If true, the intervals will be multiplied by a factor of [1,2]. Defaults to true.
  • Increase unit test coverage (retry.test.ts)
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
changeset-botBot
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

🦋 Changeset detected
Latest commit: c48f5d3
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 20 packages
NameType
@clerk/sharedMajor
@clerk/agent-toolkitPatch
@clerk/astroPatch
@clerk/backendPatch
@clerk/chrome-extensionPatch
@clerk/clerk-jsPatch
@clerk/elementsPatch
@clerk/expo-passkeysPatch
@clerk/clerk-expoPatch
@clerk/expressPatch
@clerk/fastifyPatch
@clerk/nextjsPatch
@clerk/nuxtPatch
@clerk/react-routerPatch
@clerk/clerk-reactPatch
@clerk/remixPatch
@clerk/tanstack-startPatch
@clerk/testingPatch
@clerk/uiPatch
@clerk/vuePatch
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
vercelBot
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎
NameStatusPreviewCommentsUpdated (UTC)
clerk-js-sandbox🛑 Canceled (Inspect)Feb 14, 2025 0:51am
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
110a8c3 to
f0aa6edCompareFebruary 13, 2025 11:21
@nikosdouvlis
nikosdouvlis
commented
Feb 13, 2025

Copy link


Copy Markdown

MemberAuthor
!allow-major
panteliselef
panteliselef
reviewed
Feb 13, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
Outdated

LauraBeatris
LauraBeatris
reviewed
Feb 13, 2025
View reviewed changes
Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

anagstef
anagstef
reviewed
Feb 13, 2025
View reviewed changes
Comment thread.changeset/metal-bobcats-wash.md
Outdated

LekoArts
LekoArts
suggested changes
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
Outdated

Comment threadpackages/shared/src/retry.ts
Outdated

Comment threadpackages/shared/src/retry.ts
Comment threadpackages/shared/src/retry.ts
Outdated


Comment on lines

+32
 to 
+37

 retryImmediatelyOnce: boolean;
 /**
 * The delay for the immediate retry.
 * @default 100
 */
 retryImmediatelyDelay: Milliseconds;
@LekoArtsLekoArtsFeb 14, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

These two options feel weird, I can't put a finger on it but this seems like something that should be only one option and be renamed.
  • Retrying immediately already implies that it can only happen once, right? Afterwards it's not immediate anymore
  • Technically "immediate" would be a delay of 0 so the current default of 100 is already quite arbitrary. Why should we need to change this from a case by case basis?
So I'd just put those two options into one called retryImmediate: boolean
@nikosdouvlisnikosdouvlisFeb 14, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Tagging @panteliselef as he was also interested about this - Im also open to suggestions to improve this further.

There are at least two cases where we might want to immediately retry, before applying the backoff strategy:
  • mission-critical operations that need to be completed as fast as possible
  • transient errors on the network level (think about mobile devices)
This property decouples the immediate retry with the rest of the backoff logic, as its very hard to tune the options using a combination of factor + initialDelay otherwise.
@nikosdouvlisnikosdouvlisFeb 14, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I'm going to merge these two into a single retryImmediately: boolean with a default delay of 100ms. If we ever need to change this, we can add a new prop then
@nikosdouvlisnikosdouvlisFeb 14, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done
Comment threadpackages/shared/src/retry.ts
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
f0aa6ed to
6dc4e5dCompareFebruary 14, 2025 11:22
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
6dc4e5d to
b0646a7CompareFebruary 14, 2025 11:30
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
b0646a7 to
b1f5bc5CompareFebruary 14, 2025 11:42
panteliselef
panteliselef
approved these changes
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
LekoArts
LekoArts
reviewed
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

nikosdouvlisand others
added 3 commits
February 14, 2025 14:47
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
3f3284a
Co-authored-by: Lennart <lekoarts@gmail.com>
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
6e9fb24
Co-authored-by: Lennart <lekoarts@gmail.com>
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
c48f5d3
Co-authored-by: Lennart <lekoarts@gmail.com>
brkalow
brkalow
reviewed
Feb 18, 2025
View reviewed changes
Comment threadpackages/backend/src/tokens/keys.ts
 }
 const fetcher = () => fetchJWKSFromBAPI(apiUrl, secretKey, apiVersion);
 const { keys } = await callWithRetry<{ keys: JsonWebKeyWithKid[] }>(fetcher);
 const fetcher = () => fetchJWKSFromBAPI(apiUrl, secretKey, apiVersion) as Promise<{ keys: JsonWebKeyWithKid[] }>;
@brkalowbrkalowFeb 18, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

❓ why do we need to cast here?
brkalow
brkalow
approved these changes
Feb 18, 2025
View reviewed changes
@brkalowbrkalow
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

👏
@nikosdouvlis
nikosdouvlis
merged commit 7ae77b7
into
mainFeb 25, 2025
@nikosdouvlis
nikosdouvlis
deleted the

nikos/retry-errors-in-clerkjs

branch
February 25, 2025 09:14
@clerk-cookieclerk-cookie
mentioned this pull request
Feb 25, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@brkalowbrkalowbrkalow approved these changes

+4 more reviewers

@LauraBeatrisLauraBeatrisLauraBeatris left review comments
@anagstefanagstefanagstef approved these changes
@LekoArtsLekoArtsLekoArts approved these changes
@panteliselefpanteliselefpanteliselef approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

backendclerk-js

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@nikosdouvlis@brkalow@anagstef@LekoArts@panteliselef@LauraBeatris@clerk-cookie

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
feat(shared): Replace callWithRetry and runWithExponentialBackoff with retry - #5144
Merged
nikosdouvlis merged 4 commits into
mainfrom 
nikos/retry-errors-in-clerkjs
Feb 25, 2025
Merged
feat(shared): Replace callWithRetry and runWithExponentialBackoff with retry#5144
nikosdouvlis merged 4 commits into
mainfrom 
nikos/retry-errors-in-clerkjs
Conversation
@nikosdouvlis
@nikosdouvlisnikosdouvlis
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Description
  • Replace callWithRetry with runWithExponentialBackOff so we only have a single retry mechanism in use
  • Rename runWithExponentialBackOff to retry for clarity
  • Add the following config options to runWithExponentialBackOff :

    • retryImmediately (boolean): Controls whether the helper should retry the operation immediately. If true, the exponential backoff delay will start only once this retry is over. Defaults to true.
    • jitter (boolean): If true, the intervals will be multiplied by a factor of [1,2]. Defaults to true.
  • Increase unit test coverage (retry.test.ts)
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
changeset-botBot
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

🦋 Changeset detected
Latest commit: c48f5d3
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 20 packages
NameType
@clerk/sharedMajor
@clerk/agent-toolkitPatch
@clerk/astroPatch
@clerk/backendPatch
@clerk/chrome-extensionPatch
@clerk/clerk-jsPatch
@clerk/elementsPatch
@clerk/expo-passkeysPatch
@clerk/clerk-expoPatch
@clerk/expressPatch
@clerk/fastifyPatch
@clerk/nextjsPatch
@clerk/nuxtPatch
@clerk/react-routerPatch
@clerk/clerk-reactPatch
@clerk/remixPatch
@clerk/tanstack-startPatch
@clerk/testingPatch
@clerk/uiPatch
@clerk/vuePatch
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
vercelBot
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎
NameStatusPreviewCommentsUpdated (UTC)
clerk-js-sandbox🛑 Canceled (Inspect)Feb 14, 2025 0:51am
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
110a8c3 to
f0aa6edCompareFebruary 13, 2025 11:21
@nikosdouvlis
nikosdouvlis
commented
Feb 13, 2025

Copy link


Copy Markdown

MemberAuthor
!allow-major
panteliselef
panteliselef
reviewed
Feb 13, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
Outdated

LauraBeatris
LauraBeatris
reviewed
Feb 13, 2025
View reviewed changes
Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

anagstef
anagstef
reviewed
Feb 13, 2025
View reviewed changes
Comment thread.changeset/metal-bobcats-wash.md
Outdated

LekoArts
LekoArts
suggested changes
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
Outdated

Comment threadpackages/shared/src/retry.ts
Outdated

Comment threadpackages/shared/src/retry.ts
Comment threadpackages/shared/src/retry.ts
Outdated


Comment on lines

+32
 to 
+37

 retryImmediatelyOnce: boolean;
 /**
 * The delay for the immediate retry.
 * @default 100
 */
 retryImmediatelyDelay: Milliseconds;
@LekoArtsLekoArtsFeb 14, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

These two options feel weird, I can't put a finger on it but this seems like something that should be only one option and be renamed.
  • Retrying immediately already implies that it can only happen once, right? Afterwards it's not immediate anymore
  • Technically "immediate" would be a delay of 0 so the current default of 100 is already quite arbitrary. Why should we need to change this from a case by case basis?
So I'd just put those two options into one called retryImmediate: boolean
@nikosdouvlisnikosdouvlisFeb 14, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Tagging @panteliselef as he was also interested about this - Im also open to suggestions to improve this further.

There are at least two cases where we might want to immediately retry, before applying the backoff strategy:
  • mission-critical operations that need to be completed as fast as possible
  • transient errors on the network level (think about mobile devices)
This property decouples the immediate retry with the rest of the backoff logic, as its very hard to tune the options using a combination of factor + initialDelay otherwise.
@nikosdouvlisnikosdouvlisFeb 14, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I'm going to merge these two into a single retryImmediately: boolean with a default delay of 100ms. If we ever need to change this, we can add a new prop then
@nikosdouvlisnikosdouvlisFeb 14, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done
Comment threadpackages/shared/src/retry.ts
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
f0aa6ed to
6dc4e5dCompareFebruary 14, 2025 11:22
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
6dc4e5d to
b0646a7CompareFebruary 14, 2025 11:30
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
b0646a7 to
b1f5bc5CompareFebruary 14, 2025 11:42
panteliselef
panteliselef
approved these changes
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
LekoArts
LekoArts
reviewed
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

nikosdouvlisand others
added 3 commits
February 14, 2025 14:47
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
3f3284a
Co-authored-by: Lennart <lekoarts@gmail.com>
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
6e9fb24
Co-authored-by: Lennart <lekoarts@gmail.com>
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
c48f5d3
Co-authored-by: Lennart <lekoarts@gmail.com>
brkalow
brkalow
reviewed
Feb 18, 2025
View reviewed changes
Comment threadpackages/backend/src/tokens/keys.ts
 }
 const fetcher = () => fetchJWKSFromBAPI(apiUrl, secretKey, apiVersion);
 const { keys } = await callWithRetry<{ keys: JsonWebKeyWithKid[] }>(fetcher);
 const fetcher = () => fetchJWKSFromBAPI(apiUrl, secretKey, apiVersion) as Promise<{ keys: JsonWebKeyWithKid[] }>;
@brkalowbrkalowFeb 18, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

❓ why do we need to cast here?
brkalow
brkalow
approved these changes
Feb 18, 2025
View reviewed changes
@brkalowbrkalow
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

👏
@nikosdouvlis
nikosdouvlis
merged commit 7ae77b7
into
mainFeb 25, 2025
@nikosdouvlis
nikosdouvlis
deleted the

nikos/retry-errors-in-clerkjs

branch
February 25, 2025 09:14
@clerk-cookieclerk-cookie
mentioned this pull request
Feb 25, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@brkalowbrkalowbrkalow approved these changes

+4 more reviewers

@LauraBeatrisLauraBeatrisLauraBeatris left review comments
@anagstefanagstefanagstef approved these changes
@LekoArtsLekoArtsLekoArts approved these changes
@panteliselefpanteliselefpanteliselef approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

backendclerk-js

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@nikosdouvlis@brkalow@anagstef@LekoArts@panteliselef@LauraBeatris@clerk-cookie

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(shared): Replace callWithRetry and runWithExponentialBackoff with retry - #5144
Merged
nikosdouvlis merged 4 commits into
mainfrom 
nikos/retry-errors-in-clerkjs
Feb 25, 2025
Merged
feat(shared): Replace callWithRetry and runWithExponentialBackoff with retry#5144
nikosdouvlis merged 4 commits into
mainfrom 
nikos/retry-errors-in-clerkjs
Conversation
@nikosdouvlis
@nikosdouvlisnikosdouvlis
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Description
  • Replace callWithRetry with runWithExponentialBackOff so we only have a single retry mechanism in use
  • Rename runWithExponentialBackOff to retry for clarity
  • Add the following config options to runWithExponentialBackOff :

    • retryImmediately (boolean): Controls whether the helper should retry the operation immediately. If true, the exponential backoff delay will start only once this retry is over. Defaults to true.
    • jitter (boolean): If true, the intervals will be multiplied by a factor of [1,2]. Defaults to true.
  • Increase unit test coverage (retry.test.ts)
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
changeset-botBot
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

🦋 Changeset detected
Latest commit: c48f5d3
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 20 packages
NameType
@clerk/sharedMajor
@clerk/agent-toolkitPatch
@clerk/astroPatch
@clerk/backendPatch
@clerk/chrome-extensionPatch
@clerk/clerk-jsPatch
@clerk/elementsPatch
@clerk/expo-passkeysPatch
@clerk/clerk-expoPatch
@clerk/expressPatch
@clerk/fastifyPatch
@clerk/nextjsPatch
@clerk/nuxtPatch
@clerk/react-routerPatch
@clerk/clerk-reactPatch
@clerk/remixPatch
@clerk/tanstack-startPatch
@clerk/testingPatch
@clerk/uiPatch
@clerk/vuePatch
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
vercelBot
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎
NameStatusPreviewCommentsUpdated (UTC)
clerk-js-sandbox🛑 Canceled (Inspect)Feb 14, 2025 0:51am
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
110a8c3 to
f0aa6edCompareFebruary 13, 2025 11:21
@nikosdouvlis
nikosdouvlis
commented
Feb 13, 2025

Copy link


Copy Markdown

MemberAuthor
!allow-major
panteliselef
panteliselef
reviewed
Feb 13, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
Outdated

LauraBeatris
LauraBeatris
reviewed
Feb 13, 2025
View reviewed changes
Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

anagstef
anagstef
reviewed
Feb 13, 2025
View reviewed changes
Comment thread.changeset/metal-bobcats-wash.md
Outdated

LekoArts
LekoArts
suggested changes
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
Outdated

Comment threadpackages/shared/src/retry.ts
Outdated

Comment threadpackages/shared/src/retry.ts
Comment threadpackages/shared/src/retry.ts
Outdated


Comment on lines

+32
 to 
+37

 retryImmediatelyOnce: boolean;
 /**
 * The delay for the immediate retry.
 * @default 100
 */
 retryImmediatelyDelay: Milliseconds;
@LekoArtsLekoArtsFeb 14, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

These two options feel weird, I can't put a finger on it but this seems like something that should be only one option and be renamed.
  • Retrying immediately already implies that it can only happen once, right? Afterwards it's not immediate anymore
  • Technically "immediate" would be a delay of 0 so the current default of 100 is already quite arbitrary. Why should we need to change this from a case by case basis?
So I'd just put those two options into one called retryImmediate: boolean
@nikosdouvlisnikosdouvlisFeb 14, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Tagging @panteliselef as he was also interested about this - Im also open to suggestions to improve this further.

There are at least two cases where we might want to immediately retry, before applying the backoff strategy:
  • mission-critical operations that need to be completed as fast as possible
  • transient errors on the network level (think about mobile devices)
This property decouples the immediate retry with the rest of the backoff logic, as its very hard to tune the options using a combination of factor + initialDelay otherwise.
@nikosdouvlisnikosdouvlisFeb 14, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I'm going to merge these two into a single retryImmediately: boolean with a default delay of 100ms. If we ever need to change this, we can add a new prop then
@nikosdouvlisnikosdouvlisFeb 14, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done
Comment threadpackages/shared/src/retry.ts
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
f0aa6ed to
6dc4e5dCompareFebruary 14, 2025 11:22
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
6dc4e5d to
b0646a7CompareFebruary 14, 2025 11:30
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
b0646a7 to
b1f5bc5CompareFebruary 14, 2025 11:42
panteliselef
panteliselef
approved these changes
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
LekoArts
LekoArts
reviewed
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

nikosdouvlisand others
added 3 commits
February 14, 2025 14:47
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
3f3284a
Co-authored-by: Lennart <lekoarts@gmail.com>
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
6e9fb24
Co-authored-by: Lennart <lekoarts@gmail.com>
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
c48f5d3
Co-authored-by: Lennart <lekoarts@gmail.com>
brkalow
brkalow
reviewed
Feb 18, 2025
View reviewed changes
Comment threadpackages/backend/src/tokens/keys.ts
 }
 const fetcher = () => fetchJWKSFromBAPI(apiUrl, secretKey, apiVersion);
 const { keys } = await callWithRetry<{ keys: JsonWebKeyWithKid[] }>(fetcher);
 const fetcher = () => fetchJWKSFromBAPI(apiUrl, secretKey, apiVersion) as Promise<{ keys: JsonWebKeyWithKid[] }>;
@brkalowbrkalowFeb 18, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

❓ why do we need to cast here?
brkalow
brkalow
approved these changes
Feb 18, 2025
View reviewed changes
@brkalowbrkalow
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

👏
@nikosdouvlis
nikosdouvlis
merged commit 7ae77b7
into
mainFeb 25, 2025
@nikosdouvlis
nikosdouvlis
deleted the

nikos/retry-errors-in-clerkjs

branch
February 25, 2025 09:14
@clerk-cookieclerk-cookie
mentioned this pull request
Feb 25, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@brkalowbrkalowbrkalow approved these changes

+4 more reviewers

@LauraBeatrisLauraBeatrisLauraBeatris left review comments
@anagstefanagstefanagstef approved these changes
@LekoArtsLekoArtsLekoArts approved these changes
@panteliselefpanteliselefpanteliselef approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

backendclerk-js

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@nikosdouvlis@brkalow@anagstef@LekoArts@panteliselef@LauraBeatris@clerk-cookie

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(shared): Replace callWithRetry and runWithExponentialBackoff with retry - #5144
Merged
nikosdouvlis merged 4 commits into
mainfrom 
nikos/retry-errors-in-clerkjs
Feb 25, 2025
Merged
feat(shared): Replace callWithRetry and runWithExponentialBackoff with retry#5144
nikosdouvlis merged 4 commits into
mainfrom 
nikos/retry-errors-in-clerkjs
Conversation
@nikosdouvlis
@nikosdouvlisnikosdouvlis
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Description
  • Replace callWithRetry with runWithExponentialBackOff so we only have a single retry mechanism in use
  • Rename runWithExponentialBackOff to retry for clarity
  • Add the following config options to runWithExponentialBackOff :

    • retryImmediately (boolean): Controls whether the helper should retry the operation immediately. If true, the exponential backoff delay will start only once this retry is over. Defaults to true.
    • jitter (boolean): If true, the intervals will be multiplied by a factor of [1,2]. Defaults to true.
  • Increase unit test coverage (retry.test.ts)
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
changeset-botBot
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

🦋 Changeset detected
Latest commit: c48f5d3
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 20 packages
NameType
@clerk/sharedMajor
@clerk/agent-toolkitPatch
@clerk/astroPatch
@clerk/backendPatch
@clerk/chrome-extensionPatch
@clerk/clerk-jsPatch
@clerk/elementsPatch
@clerk/expo-passkeysPatch
@clerk/clerk-expoPatch
@clerk/expressPatch
@clerk/fastifyPatch
@clerk/nextjsPatch
@clerk/nuxtPatch
@clerk/react-routerPatch
@clerk/clerk-reactPatch
@clerk/remixPatch
@clerk/tanstack-startPatch
@clerk/testingPatch
@clerk/uiPatch
@clerk/vuePatch
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
vercelBot
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎
NameStatusPreviewCommentsUpdated (UTC)
clerk-js-sandbox🛑 Canceled (Inspect)Feb 14, 2025 0:51am
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
110a8c3 to
f0aa6edCompareFebruary 13, 2025 11:21
@nikosdouvlis
nikosdouvlis
commented
Feb 13, 2025

Copy link


Copy Markdown

MemberAuthor
!allow-major
panteliselef
panteliselef
reviewed
Feb 13, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
Outdated

LauraBeatris
LauraBeatris
reviewed
Feb 13, 2025
View reviewed changes
Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

anagstef
anagstef
reviewed
Feb 13, 2025
View reviewed changes
Comment thread.changeset/metal-bobcats-wash.md
Outdated

LekoArts
LekoArts
suggested changes
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
Outdated

Comment threadpackages/shared/src/retry.ts
Outdated

Comment threadpackages/shared/src/retry.ts
Comment threadpackages/shared/src/retry.ts
Outdated


Comment on lines

+32
 to 
+37

 retryImmediatelyOnce: boolean;
 /**
 * The delay for the immediate retry.
 * @default 100
 */
 retryImmediatelyDelay: Milliseconds;
@LekoArtsLekoArtsFeb 14, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

These two options feel weird, I can't put a finger on it but this seems like something that should be only one option and be renamed.
  • Retrying immediately already implies that it can only happen once, right? Afterwards it's not immediate anymore
  • Technically "immediate" would be a delay of 0 so the current default of 100 is already quite arbitrary. Why should we need to change this from a case by case basis?
So I'd just put those two options into one called retryImmediate: boolean
@nikosdouvlisnikosdouvlisFeb 14, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Tagging @panteliselef as he was also interested about this - Im also open to suggestions to improve this further.

There are at least two cases where we might want to immediately retry, before applying the backoff strategy:
  • mission-critical operations that need to be completed as fast as possible
  • transient errors on the network level (think about mobile devices)
This property decouples the immediate retry with the rest of the backoff logic, as its very hard to tune the options using a combination of factor + initialDelay otherwise.
@nikosdouvlisnikosdouvlisFeb 14, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I'm going to merge these two into a single retryImmediately: boolean with a default delay of 100ms. If we ever need to change this, we can add a new prop then
@nikosdouvlisnikosdouvlisFeb 14, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done
Comment threadpackages/shared/src/retry.ts
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
f0aa6ed to
6dc4e5dCompareFebruary 14, 2025 11:22
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
6dc4e5d to
b0646a7CompareFebruary 14, 2025 11:30
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
b0646a7 to
b1f5bc5CompareFebruary 14, 2025 11:42
panteliselef
panteliselef
approved these changes
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
LekoArts
LekoArts
reviewed
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

nikosdouvlisand others
added 3 commits
February 14, 2025 14:47
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
3f3284a
Co-authored-by: Lennart <lekoarts@gmail.com>
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
6e9fb24
Co-authored-by: Lennart <lekoarts@gmail.com>
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
c48f5d3
Co-authored-by: Lennart <lekoarts@gmail.com>
brkalow
brkalow
reviewed
Feb 18, 2025
View reviewed changes
Comment threadpackages/backend/src/tokens/keys.ts
 }
 const fetcher = () => fetchJWKSFromBAPI(apiUrl, secretKey, apiVersion);
 const { keys } = await callWithRetry<{ keys: JsonWebKeyWithKid[] }>(fetcher);
 const fetcher = () => fetchJWKSFromBAPI(apiUrl, secretKey, apiVersion) as Promise<{ keys: JsonWebKeyWithKid[] }>;
@brkalowbrkalowFeb 18, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

❓ why do we need to cast here?
brkalow
brkalow
approved these changes
Feb 18, 2025
View reviewed changes
@brkalowbrkalow
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

👏
@nikosdouvlis
nikosdouvlis
merged commit 7ae77b7
into
mainFeb 25, 2025
@nikosdouvlis
nikosdouvlis
deleted the

nikos/retry-errors-in-clerkjs

branch
February 25, 2025 09:14
@clerk-cookieclerk-cookie
mentioned this pull request
Feb 25, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@brkalowbrkalowbrkalow approved these changes

+4 more reviewers

@LauraBeatrisLauraBeatrisLauraBeatris left review comments
@anagstefanagstefanagstef approved these changes
@LekoArtsLekoArtsLekoArts approved these changes
@panteliselefpanteliselefpanteliselef approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

backendclerk-js

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@nikosdouvlis@brkalow@anagstef@LekoArts@panteliselef@LauraBeatris@clerk-cookie

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
feat(shared): Replace callWithRetry and runWithExponentialBackoff with retry - #5144
Merged
nikosdouvlis merged 4 commits into
mainfrom 
nikos/retry-errors-in-clerkjs
Feb 25, 2025
Merged
feat(shared): Replace callWithRetry and runWithExponentialBackoff with retry#5144
nikosdouvlis merged 4 commits into
mainfrom 
nikos/retry-errors-in-clerkjs
Conversation
@nikosdouvlis
@nikosdouvlisnikosdouvlis
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Description
  • Replace callWithRetry with runWithExponentialBackOff so we only have a single retry mechanism in use
  • Rename runWithExponentialBackOff to retry for clarity
  • Add the following config options to runWithExponentialBackOff :

    • retryImmediately (boolean): Controls whether the helper should retry the operation immediately. If true, the exponential backoff delay will start only once this retry is over. Defaults to true.
    • jitter (boolean): If true, the intervals will be multiplied by a factor of [1,2]. Defaults to true.
  • Increase unit test coverage (retry.test.ts)
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
changeset-botBot
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

🦋 Changeset detected
Latest commit: c48f5d3
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 20 packages
NameType
@clerk/sharedMajor
@clerk/agent-toolkitPatch
@clerk/astroPatch
@clerk/backendPatch
@clerk/chrome-extensionPatch
@clerk/clerk-jsPatch
@clerk/elementsPatch
@clerk/expo-passkeysPatch
@clerk/clerk-expoPatch
@clerk/expressPatch
@clerk/fastifyPatch
@clerk/nextjsPatch
@clerk/nuxtPatch
@clerk/react-routerPatch
@clerk/clerk-reactPatch
@clerk/remixPatch
@clerk/tanstack-startPatch
@clerk/testingPatch
@clerk/uiPatch
@clerk/vuePatch
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
vercelBot
commented
Feb 13, 2025•

edited

Loading

Copy link


Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎
NameStatusPreviewCommentsUpdated (UTC)
clerk-js-sandbox🛑 Canceled (Inspect)Feb 14, 2025 0:51am
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
110a8c3 to
f0aa6edCompareFebruary 13, 2025 11:21
@nikosdouvlis
nikosdouvlis
commented
Feb 13, 2025

Copy link


Copy Markdown

MemberAuthor
!allow-major
panteliselef
panteliselef
reviewed
Feb 13, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
Outdated

LauraBeatris
LauraBeatris
reviewed
Feb 13, 2025
View reviewed changes
Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

anagstef
anagstef
reviewed
Feb 13, 2025
View reviewed changes
Comment thread.changeset/metal-bobcats-wash.md
Outdated

LekoArts
LekoArts
suggested changes
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
Outdated

Comment threadpackages/shared/src/retry.ts
Outdated

Comment threadpackages/shared/src/retry.ts
Comment threadpackages/shared/src/retry.ts
Outdated


Comment on lines

+32
 to 
+37

 retryImmediatelyOnce: boolean;
 /**
 * The delay for the immediate retry.
 * @default 100
 */
 retryImmediatelyDelay: Milliseconds;
@LekoArtsLekoArtsFeb 14, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

These two options feel weird, I can't put a finger on it but this seems like something that should be only one option and be renamed.
  • Retrying immediately already implies that it can only happen once, right? Afterwards it's not immediate anymore
  • Technically "immediate" would be a delay of 0 so the current default of 100 is already quite arbitrary. Why should we need to change this from a case by case basis?
So I'd just put those two options into one called retryImmediate: boolean
@nikosdouvlisnikosdouvlisFeb 14, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Tagging @panteliselef as he was also interested about this - Im also open to suggestions to improve this further.

There are at least two cases where we might want to immediately retry, before applying the backoff strategy:
  • mission-critical operations that need to be completed as fast as possible
  • transient errors on the network level (think about mobile devices)
This property decouples the immediate retry with the rest of the backoff logic, as its very hard to tune the options using a combination of factor + initialDelay otherwise.
@nikosdouvlisnikosdouvlisFeb 14, 2025•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I'm going to merge these two into a single retryImmediately: boolean with a default delay of 100ms. If we ever need to change this, we can add a new prop then
@nikosdouvlisnikosdouvlisFeb 14, 2025

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done
Comment threadpackages/shared/src/retry.ts
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
f0aa6ed to
6dc4e5dCompareFebruary 14, 2025 11:22
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
6dc4e5d to
b0646a7CompareFebruary 14, 2025 11:30
@nikosdouvlis
nikosdouvlisforce-pushed
the
nikos/retry-errors-in-clerkjs
branch
from
b0646a7 to
b1f5bc5CompareFebruary 14, 2025 11:42
panteliselef
panteliselef
approved these changes
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/retry.ts
LekoArts
LekoArts
reviewed
Feb 14, 2025
View reviewed changes
Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

Comment threadpackages/shared/src/__tests__/retry.test.ts
Outdated

nikosdouvlisand others
added 3 commits
February 14, 2025 14:47
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
3f3284a
Co-authored-by: Lennart <lekoarts@gmail.com>
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
6e9fb24
Co-authored-by: Lennart <lekoarts@gmail.com>
@nikosdouvlis@LekoArts
Update packages/shared/src/__tests__/retry.test.ts
c48f5d3
Co-authored-by: Lennart <lekoarts@gmail.com>
brkalow
brkalow
reviewed
Feb 18, 2025
View reviewed changes
Comment threadpackages/backend/src/tokens/keys.ts
 }
 const fetcher = () => fetchJWKSFromBAPI(apiUrl, secretKey, apiVersion);
 const { keys } = await callWithRetry<{ keys: JsonWebKeyWithKid[] }>(fetcher);
 const fetcher = () => fetchJWKSFromBAPI(apiUrl, secretKey, apiVersion) as Promise<{ keys: JsonWebKeyWithKid[] }>;
@brkalowbrkalowFeb 18, 2025

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

❓ why do we need to cast here?
brkalow
brkalow
approved these changes
Feb 18, 2025
View reviewed changes
@brkalowbrkalow
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

👏
@nikosdouvlis
nikosdouvlis
merged commit 7ae77b7
into
mainFeb 25, 2025
@nikosdouvlis
nikosdouvlis
deleted the

nikos/retry-errors-in-clerkjs

branch
February 25, 2025 09:14
@clerk-cookieclerk-cookie
mentioned this pull request
Feb 25, 2025
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@brkalowbrkalowbrkalow approved these changes

+4 more reviewers

@LauraBeatrisLauraBeatrisLauraBeatris left review comments
@anagstefanagstefanagstef approved these changes
@LekoArtsLekoArtsLekoArts approved these changes
@panteliselefpanteliselefpanteliselef approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

backendclerk-js

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@nikosdouvlis@brkalow@anagstef@LekoArts@panteliselef@LauraBeatris@clerk-cookie