Skip to content

fix(clerk-js): Improve logging for CAPTCHA script loading errors #3374

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

Merged
anagstef merged 3 commits into from
May 13, 2024
Merged

fix(clerk-js): Improve logging for CAPTCHA script loading errors #3374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b5f9811
fix(clerk-js): Improve logging for CAPTCHA script loading errors
May 13, 2024
3984bd9
fix(clerk-js): Remove the generic CAPTCHA console error
May 13, 2024
e30bde7
Merge branch 'main' into stefanos/core-2247-investigate-if-we-can-exp…
anagstef May 13, 2024
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
5 changes: 5 additions & 0 deletions .changeset/fast-timers-matter.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@clerk/clerk-js': patch
---

Improve logging for CAPTCHA script loading errors
11 changes: 6 additions & 5 deletions packages/clerk-js/src/utils/captcha.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import { loadScript } from '@clerk/shared/loadScript';
import type { CaptchaWidgetType } from '@clerk/types';

import { clerkFailedToLoadThirdPartyScript } from '../core/errors';

interface RenderOptions {
/**
* Every widget has a sitekey. This sitekey is associated with the corresponding widget configuration and is created upon the widget creation.
Expand Down Expand Up @@ -76,9 +74,12 @@ export async function loadCaptcha(url: string) {
if (!window.turnstile) {
try {
await loadScript(url, { defer: true });
} catch (_) {
} catch {
// Rethrow with specific message
clerkFailedToLoadThirdPartyScript('Cloudflare Turnstile');

@dimkl dimkl May 13, 2024

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.

🙃 Why did we drop the usage of clerkFailedToLoadThirdPartyScript ? Couldn't we enhance it with an optional url parameter?

@anagstef anagstef May 13, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As described above, the clerkFailedToLoadThirdPartyScript throws an Error object which is not what we want to do here.

console.error('Clerk: Failed to load the CAPTCHA script from the URL: ', url);
throw {
captchaError: 'captcha_script_failed_to_load',
};

@dimkl dimkl May 13, 2024

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.

🙃 I would suggest we throw a ClerkError (an existing one or create a new one) instead of throwing an object. In order to provide a better error handling in the future i think that we will need to use our Error classes.

@anagstef anagstef May 13, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's a bit complicated! We don't want everything to throw here. We actually send the captchaError error value to the FAPI.

}
}
return window.turnstile;
Expand Down Expand Up @@ -112,7 +113,7 @@ export const getCaptchaToken = async (captchaOptions: {
return div;
};

const captcha = await loadCaptcha(scriptUrl);
const captcha: Turnstile = await loadCaptcha(scriptUrl);
let retries = 0;
const errorCodes: (string | number)[] = [];

Expand Down