-
Notifications
You must be signed in to change notification settings - Fork 433
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?
feat(ui): enforce minimum clerk-js version compatibility #7667
Conversation
🦋 Changeset detectedLatest commit: 38deb49 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
f6578d0 to
682ea78
Compare
682ea78 to
a033dda
Compare
a033dda to
d1d402b
Compare
packages/ui/src/constants.ts
Outdated
| @@ -1,3 +1,5 @@ | |||
| export const MIN_CLERK_JS_VERSION = '5.112.0'; | |||
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.
Is 5.112.0 the correct version here or should this be pinned to 6.0.0 since thats the major for core 3? The tests seem to use 6.0.0 as the expected minimum which made me wonder if this was intentional or if the constant just needs updating.
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.
I think the version when we added ModuleManager/ClerkUi was 5.112.0 so in theory this could be the minimum clerk js version. But we can also arbitrarily set this to 6.0.0 to make it available only on Core3. WDYT?
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.
I updated this to 6.0.0 73ebcb5
| logger.warnOnce(incompatibilityMessage); | ||
| } else { | ||
| throw new ClerkRuntimeError(incompatibilityMessage, { code: 'clerk_ui_version_mismatch' }); | ||
| } |
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.
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).
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.
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
| `@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.`; | ||
| } |
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(), and clerkVersion comes 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/ClerkUi and I don't think you land in this case right now, but you never know in the future.
|
This is fine |
📝 WalkthroughWalkthroughThis pull request adds runtime Clerk.js SDK version validation to 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Add runtime version check in ClerkUi constructor to detect incompatible @clerk/clerk-js versions. In development instances, logs a warning; in production, throws ClerkRuntimeError with actionable upgrade guidance.
4e1751d to
73ebcb5
Compare
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
…kjs-sdk-version-in-clerkui
…kjs-sdk-version-in-clerkui
Add runtime version check in ClerkUi constructor to detect incompatible @clerk/clerk-js versions. In development instances, logs a warning; in production, throws ClerkRuntimeError with actionable upgrade guidance.
Also moves shared version utilities to @clerk/shared/utils.
Description
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Tests