-
Notifications
You must be signed in to change notification settings - Fork 431
feat(ui): enforce minimum clerk-js version compatibility #7667
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,12 @@ | ||
| import { ClerkRuntimeError } from '@clerk/shared/error'; | ||
| import { logger } from '@clerk/shared/logger'; | ||
| import type { ModuleManager } from '@clerk/shared/moduleManager'; | ||
| import type { Clerk, ClerkOptions, EnvironmentResource } from '@clerk/shared/types'; | ||
| import type { ClerkUiInstance, ComponentControls as SharedComponentControls } from '@clerk/shared/ui'; | ||
| import { isVersionAtLeast, parseVersion } from '@clerk/shared/versionCheck'; | ||
|
|
||
| import { type MountComponentRenderer, mountComponentRenderer } from './Components'; | ||
| import { MIN_CLERK_JS_VERSION } from './constants'; | ||
|
|
||
| export class ClerkUi implements ClerkUiInstance { | ||
| static version = __PKG_VERSION__; | ||
|
|
@@ -16,6 +20,33 @@ export class ClerkUi implements ClerkUiInstance { | |
| options: ClerkOptions, | ||
| moduleManager: ModuleManager, | ||
| ) { | ||
| const clerk = getClerk(); | ||
| const clerkVersion = clerk?.version; | ||
| const isDevelopmentInstance = clerk?.instanceType === 'development'; | ||
| const parsedVersion = parseVersion(clerkVersion ?? ''); | ||
|
|
||
| let incompatibilityMessage: string | null = null; | ||
|
|
||
| if (parsedVersion && !isVersionAtLeast(clerkVersion, MIN_CLERK_JS_VERSION)) { | ||
| incompatibilityMessage = | ||
| `@clerk/ui@${ClerkUi.version} requires @clerk/clerk-js@>=${MIN_CLERK_JS_VERSION}, ` + | ||
| `but found @clerk/clerk-js@${clerkVersion}. ` + | ||
| `Please upgrade @clerk/clerk-js (or your framework SDK) to a compatible version.`; | ||
| } else if (!parsedVersion && !moduleManager) { | ||
| incompatibilityMessage = | ||
| `@clerk/ui@${ClerkUi.version} requires @clerk/clerk-js@>=${MIN_CLERK_JS_VERSION}, ` + | ||
| `but found an incompatible version${clerkVersion ? ` (${clerkVersion})` : ''}. ` + | ||
| `Please upgrade @clerk/clerk-js (or your framework SDK) to a compatible version.`; | ||
| } | ||
|
|
||
| if (incompatibilityMessage) { | ||
| if (isDevelopmentInstance) { | ||
| throw new ClerkRuntimeError(incompatibilityMessage, { code: 'clerk_ui_version_mismatch' }); | ||
| } else { | ||
| logger.warnOnce(incompatibilityMessage); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be the other way around? Typically we want to fail fast in development (so devs catch issues early) but be more lenient in production (to avoid breaking user-facing apps). The current behavior warns in dev (easy to miss) but throws in prod (could cause outages).
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that makes more sense. I went with "You don't want a mismatch on production" as you may end up with weird-looking UI, but I agree. 4e1751d |
||
| } | ||
|
|
||
| this.#componentRenderer = mountComponentRenderer(getClerk, getEnvironment, options, moduleManager); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at clerk-js, it always instantiates ClerkUi with
new ModuleManager(), andclerkVersioncomes from the Clerk instance which should always have its version set. Is there a scenario where ClerkUi would be instantiated without a moduleManager or without knowing the clerkVersion? If this is just defensive coding thats fine, but if theres an actual use case Im curious what it is :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just defensive to future-proof us from a mistake. I thought about all different scenarios with pinned/hotloaded clerk-js and before/after we introduced
moduleManager/ClerkUiand I don't think you land in this case right now, but you never know in the future.