Skip to content

feat(shared): Intelligent retries for existing clerk-js script #6860

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat(shared): Intelligent retries for existing clerk-js script #6860

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9cae993
Intelligent retries for existing clerk-js script
brkalow Sep 26, 2025
7d0b4fe
remove comments
brkalow Sep 26, 2025
6b3efd1
tweak heuristic
brkalow Sep 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 60 additions & 11 deletions packages/shared/src/loadClerkJsScript.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,44 @@ function isClerkProperlyLoaded(): boolean {
return typeof clerk === 'object' && typeof clerk.load === 'function';
}

/**
* Checks if an existing script has a request error using Performance API.
*
* @param scriptUrl - The URL of the script to check.
* @returns True if the script has failed to load due to a network/HTTP error.
*/
function hasScriptRequestError(scriptUrl: string): boolean {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a slightly cursed way trying to check for an existing script load error without the ability to add an onError callback to the script. window.performance.getEntries() should contain the network request to fetch clerk-js, and so we can better infer what the status is without an arbitrary timeout.

Copy link
Member

Choose a reason for hiding this comment

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

I like this!

if (typeof window === 'undefined' || !window.performance) {
return false;
}

const entries = performance.getEntries() as PerformanceResourceTiming[];
const scriptEntry = entries.find(entry => entry.name === scriptUrl);

if (!scriptEntry) {
return false;
}

// transferSize === 0 with responseEnd === 0 indicates network failure
// transferSize === 0 with responseEnd > 0 might be a 4xx/5xx error or blocked request
if (scriptEntry.transferSize === 0 && scriptEntry.decodedBodySize === 0) {
// If there was no response at all, it's definitely an error
if (scriptEntry.responseEnd === 0) {
return true;
}
// If we got a response but no content, likely an HTTP error (4xx/5xx)
if (scriptEntry.responseEnd > 0 && scriptEntry.responseStart > 0) {
return true;
}

if (scriptEntry.responseStatus === 0) {
return true;
}
}

return false;
}
Comment on lines +62 to +98
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Fix TS type error and reduce false positives in Performance heuristics

  • responseStatus is not part of PerformanceResourceTiming; this breaks TS.
  • transferSize/decodedBodySize being 0 is common for cross-origin without TAO and is not an error signal by itself.

Refactor to use getEntriesByName and only treat the “no response observed” case as failure.

-function hasScriptRequestError(scriptUrl: string): boolean {
-  if (typeof window === 'undefined' || !window.performance) {
-    return false;
-  }
-
-  const entries = performance.getEntries() as PerformanceResourceTiming[];
-  const scriptEntry = entries.find(entry => entry.name === scriptUrl);
-
-  if (!scriptEntry) {
-    return false;
-  }
-
-  // transferSize === 0 with responseEnd === 0 indicates network failure
-  // transferSize === 0 with responseEnd > 0 might be a 4xx/5xx error or blocked request
-  if (scriptEntry.transferSize === 0 && scriptEntry.decodedBodySize === 0) {
-    // If there was no response at all, it's definitely an error
-    if (scriptEntry.responseEnd === 0) {
-      return true;
-    }
-    // If we got a response but no content, likely an HTTP error (4xx/5xx)
-    if (scriptEntry.responseEnd > 0 && scriptEntry.responseStart > 0) {
-      return true;
-    }
-
-    if (scriptEntry.responseStatus === 0) {
-      return true;
-    }
-  }
-
-  return false;
-}
+function hasScriptRequestError(scriptUrl: string): boolean {
+  if (typeof window === 'undefined' || !('performance' in window)) {
+    return false;
+  }
+  const entries = performance.getEntriesByName(scriptUrl, 'resource') as PerformanceEntry[];
+  const scriptEntry = entries.find((e): e is PerformanceResourceTiming => {
+    return (e as PerformanceResourceTiming).entryType === 'resource';
+  });
+  if (!scriptEntry) {
+    return false;
+  }
+  // Clear-cut failure: no response observed. Sizes may be 0 cross-origin even on success (no TAO).
+  return scriptEntry.responseEnd === 0;
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/**
* Checks if an existing script has a request error using Performance API.
*
* @param scriptUrl - The URL of the script to check.
* @returns True if the script has failed to load due to a network/HTTP error.
*/
function hasScriptRequestError(scriptUrl: string): boolean {
if (typeof window === 'undefined' || !window.performance) {
return false;
}
const entries = performance.getEntries() as PerformanceResourceTiming[];
const scriptEntry = entries.find(entry => entry.name === scriptUrl);
if (!scriptEntry) {
return false;
}
// transferSize === 0 with responseEnd === 0 indicates network failure
// transferSize === 0 with responseEnd > 0 might be a 4xx/5xx error or blocked request
if (scriptEntry.transferSize === 0 && scriptEntry.decodedBodySize === 0) {
// If there was no response at all, it's definitely an error
if (scriptEntry.responseEnd === 0) {
return true;
}
// If we got a response but no content, likely an HTTP error (4xx/5xx)
if (scriptEntry.responseEnd > 0 && scriptEntry.responseStart > 0) {
return true;
}
if (scriptEntry.responseStatus === 0) {
return true;
}
}
return false;
}
/**
* Checks if an existing script has a request error using Performance API.
*
* @param scriptUrl - The URL of the script to check.
* @returns True if the script has failed to load due to a network/HTTP error.
*/
function hasScriptRequestError(scriptUrl: string): boolean {
if (typeof window === 'undefined' || !('performance' in window)) {
return false;
}
const entries = performance.getEntriesByName(scriptUrl, 'resource') as PerformanceEntry[];
const scriptEntry = entries.find((e): e is PerformanceResourceTiming => {
return (e as PerformanceResourceTiming).entryType === 'resource';
});
if (!scriptEntry) {
return false;
}
// Clear-cut failure: no response observed. Sizes may be 0 cross-origin even on success (no TAO).
return scriptEntry.responseEnd === 0;
}
🤖 Prompt for AI Agents 
In packages/shared/src/loadClerkJsScript.ts around lines 62 to 98, the
implementation uses properties not present on PerformanceResourceTiming
(responseStatus) and treats transferSize/decodedBodySize === 0 as an error which
causes false positives for cross-origin loads; change to use
performance.getEntriesByName(scriptUrl) (typed as PerformanceResourceTiming[])
to get the relevant entries, pick the most recent entry (if any), and only treat
the script as failed when no response was observed (entry.responseEnd === 0);
remove checks for transferSize/decodedBodySize and responseStatus to fix the TS
error and reduce false positives.


/**
* Waits for Clerk to be properly loaded with a timeout mechanism.
* Uses polling to check if Clerk becomes available within the specified timeout.
Expand Down Expand Up @@ -117,11 +155,13 @@ function waitForClerkWithTimeout(timeoutMs: number): Promise<HTMLScriptElement |
}

/**
* Hotloads the Clerk JS script with robust failure detection.
* Hotloads the Clerk JS script with robust failure detection and retry logic.
*
* For existing scripts:
* - If no request error detected: waits for timeout, then retries with loadScript if timeout expires
* - If request error detected: immediately retries with loadScript.
*
* Uses a timeout-based approach to ensure absolute certainty about load success/failure.
* If the script fails to load within the timeout period, or loads but doesn't create
* a proper Clerk instance, the promise rejects with an error.
* For new scripts: uses loadScript which has built-in retry logic via the retry utility.
*
* @param opts - The options used to build the Clerk JS script URL and load the script.
* Must include a `publishableKey` if no existing script is found.
Expand All @@ -144,20 +184,29 @@ const loadClerkJsScript = async (opts?: LoadClerkJsScriptOptions): Promise<HTMLS
return null;
}

const existingScript = document.querySelector<HTMLScriptElement>('script[data-clerk-js-script]');

if (existingScript) {
return waitForClerkWithTimeout(timeout);
}

if (!opts?.publishableKey) {
errorThrower.throwMissingPublishableKeyError();
return null;
}

Comment on lines 187 to 191
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Don’t require publishableKey when a direct clerkJSUrl is provided

This throws even when a script URL is explicitly supplied. Allow either pk or URL.

-  if (!opts?.publishableKey) {
+  if (!opts?.publishableKey && !opts?.clerkJSUrl) {
     errorThrower.throwMissingPublishableKeyError();
     return null;
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!opts?.publishableKey) {
errorThrower.throwMissingPublishableKeyError();
return null;
}
if (!opts?.publishableKey && !opts?.clerkJSUrl) {
errorThrower.throwMissingPublishableKeyError();
return null;
}
🤖 Prompt for AI Agents 
In packages/shared/src/loadClerkJsScript.ts around lines 187 to 191, the current
check unconditionally throws a missing publishableKey error even when a direct
clerkJSUrl is supplied; change the guard to only require opts.publishableKey
when opts.clerkJSUrl is not provided (e.g., if neither publishableKey nor
clerkJSUrl exist then throw), keeping the existing
errorThrower.throwMissingPublishableKeyError() path for the
publishableKey-missing case and returning null as before; ensure the condition
handles undefined/null values safely (opts may be undefined) so supplying a
clerkJSUrl bypasses the publishableKey requirement.

const scriptUrl = clerkJsScriptUrl(opts);
const existingScript = document.querySelector<HTMLScriptElement>('script[data-clerk-js-script]');

if (existingScript) {
if (hasScriptRequestError(scriptUrl)) {
existingScript.remove();
} else {
try {
return await waitForClerkWithTimeout(timeout);
Copy link
Member

Choose a reason for hiding this comment

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

I'm having seconds thoughts about this particular return here. I checked the code and it seems that there's no retry logic in IsomorphicClerk (the calling site) or in loadClerkJsScript - the retry logic is in loadScript which will not be called if we return here.

So, if I understand correctly, if the <script> fails to load and we're going to remove it and implicitly retry via loadScript. If, however, the script hasn't errored at the point of the check, we're going to wait for a timeout and then completely fail. This is good because the status will change, but it feels like we should at least trigger a few retries first before failing completely. Failing to load the script cannot be recovered from the perspective of the user/dev once status switches so retrying makes sense

Copy link
Member Author

Choose a reason for hiding this comment

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

@nikosdouvlis I believe this still works because of return await. The function will not return until the promise from waitForClerkWithTimeout is resolved, which means it should throw if we can't detect that Clerk loaded in time.

I did spend some time trying to refactor this part on Friday though, I think it could be clearer what's going on. I will revisit it again today!

Copy link
Member

@nikosdouvlis nikosdouvlis Sep 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I haven't tested it locally yet, but what you're saying makes sense! The current implementation just bails and does not retry if the existing script fails to load, your fix is a nice improvement!

The await part is tricky though, if someone accidentally removes the keyword, retrying for this case will break and TS won't catch it as the return signature will not change (still a promise). Switching to then().catch might be a good first step towards making this a bit harder to break?

Another idea would be to move the retry logic from loadScript on level up (inside loadClerkJsScript) so its a little easier to understand who's responsible to handle the error thrown, but Im not 100% sold on that idea yet

} catch {
existingScript.remove();
}
}
}

const loadPromise = waitForClerkWithTimeout(timeout);

loadScript(clerkJsScriptUrl(opts), {
loadScript(scriptUrl, {
async: true,
crossOrigin: 'anonymous',
nonce: opts.nonce,
Expand Down
Loading