-
Notifications
You must be signed in to change notification settings - Fork 436
feat(react-router): Add support for keyless mode #7794
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: cda1649 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
@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: |
📝 WalkthroughWalkthroughAdds keyless support to the React Router package: server-side keyless service with file-backed storage and lazy singleton init; shared resolveKeysWithKeylessFallback helper and KeylessResult type; middleware and request-state propagation of keyless URLs; client provider propagation of internal keyless URLs; feature flag canUseKeyless and relaxed loadOptions validation to allow keyless in development; new file storage factory, utilities, and Playwright integration tests plus shared test helpers for keyless flows. Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
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.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/react-router/src/server/keyless/index.ts`:
- Around line 56-76: The current implementations of createAccountlessApplication
and completeOnboarding use a unsafe "{} as any" fallback when args is missing
and also capture args/options in the surrounding closure, which can cause
runtime errors and stale singletons. Fix by removing the "{} as any" fallback
and either (A) make the args parameter required on the public API for
createAccountlessApplication and completeOnboarding, or (B) construct a safe
keyless args object (e.g., build a minimal shape containing any properties
clerkClient expects) and pass that to clerkClient; also ensure you instantiate
clerkClient inside each method (not captured once in the outer scope) so each
call uses the provided args/options; update references to clerkClient, args,
options, createAccountlessApplication, completeOnboarding and
__experimental_accountlessApplications accordingly.
In `@packages/react-router/src/server/keyless/utils.ts`:
- Around line 77-84: The current check uses `if (!publishableKey || !secretKey)`
which can mix a configured key with a keyless key and break auth; change the
logic so you only enter keyless mode when both keys are absent (i.e., require
`!publishableKey && !secretKey`) before calling
`keylessService.getOrCreateKeys()` and assigning `publishableKey`, `secretKey`,
`claimUrl`, and `apiKeysUrl` from `keylessApp`; this ensures both
`publishableKey` and `secretKey` come from the same `keylessApp` source instead
of being mixed with configured values.
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.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@integration/testUtils/index.ts`:
- Around line 90-91: Remove the barrel re-export of
mockClaimedInstanceEnvironmentCall from integration/testUtils/index.ts (the
export line exporting mockClaimedInstanceEnvironmentCall) and update all test
call sites to import mockClaimedInstanceEnvironmentCall directly from
integration/testUtils/keylessHelpers; specifically change imports in
integration/tests/next-quickstart-keyless.test.ts,
integration/tests/react-router/keyless.test.ts, and
integration/tests/tanstack-start/keyless.test.ts to import {
mockClaimedInstanceEnvironmentCall } from './keylessHelpers' (or the correct
relative path into testUtils/keylessHelpers) so no code relies on the index.ts
re-export and to avoid circular dependencies.
In `@integration/testUtils/keylessHelpers.ts`:
- Around line 7-19: The exported helper mockClaimedInstanceEnvironmentCall is
missing an explicit TypeScript return type; update its signature to explicitly
return Promise<void> (e.g., annotate the async function as (page: Page):
Promise<void>) so the exported symbol has a concrete type, and ensure the Page
type is imported/available where mockClaimedInstanceEnvironmentCall is declared.
| export async function createFileStorage(options: FileStorageOptions = {}): Promise<KeylessStorage> { | ||
| const { cwd = () => process.cwd() } = options; | ||
|
|
||
| try { | ||
| const [fs, path] = await Promise.all([import('node:fs'), import('node:path')]); | ||
|
|
||
| return createNodeFileStorage(fs, path, { | ||
| cwd, | ||
| frameworkPackageName: '@clerk/react-router', | ||
| }); | ||
| } catch { | ||
| throw new Error( | ||
| 'Keyless mode requires a Node.js runtime with file system access. ' + | ||
| 'Set VITE_CLERK_KEYLESS_DISABLED=1 to disable keyless mode.', | ||
| ); | ||
| } | ||
| } |
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.
This can be extracted as well, but will plan to do so while implementing keyless for other SDKs.
Having these node imports in the shared @clerk/shared/keyless barrel export would break Next.js at edge runtime, since packages/nextjs/src/server/keyless-node.ts imports from that barrel (correct me if Im wrong!)
| * @param canUseKeyless - Whether keyless mode is enabled in the current environment | ||
| * @returns The resolved keys (either configured or from keyless mode) | ||
| */ | ||
| export async function resolveKeysWithKeylessFallback( |
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.
Consider making this a method on the keyless service itself, instead of passing it in as an argument
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.
Updated it to be a method on the keyless service instead of a standalone function 👍🏼
Also, we want keyless to work on Node.js runtime only, right?
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.
@wobsoriano eventually I'd like to see keyless work in the browser for SPAs, so I don't want to couple this to Node.
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.
gotcha, the current implementation should be agnostic then.
…eyless service instead of a standalone function
Uses the testToggleCollapsePopoverAndClaim helper function instead of inline implementation for consistency with other framework tests.
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.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@integration/tests/tanstack-start/keyless.test.ts`:
- Around line 41-43: The call to testToggleCollapsePopoverAndClaim is missing
the required framework parameter; update the invocation of
testToggleCollapsePopoverAndClaim({ page, context, app, dashboardUrl }) to
include framework (e.g. framework: framework or framework: '<your-framework>')
so the helper receives the framework value used in its URL validation logic;
ensure the framework identifier you pass matches the existing framework
variable/constant used elsewhere in the test suite.
In `@packages/shared/src/keyless/index.ts`:
- Around line 12-16: The barrel file exports KeylessResult twice which causes
TS2308; remove the duplicate type re-export from the
resolveKeysWithKeylessFallback export group so KeylessResult is only exported
once (keep the export type from './service'), i.e. update the exports involving
resolveKeysWithKeylessFallback to export only the value
(resolveKeysWithKeylessFallback) and not re-export KeylessResult.
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/shared/src/keyless/index.ts`:
- Around line 15-16: Remove the new barrel re-exports: delete the lines
exporting resolveKeysWithKeylessFallback and the KeylessResult type from the
index.ts barrel; instead update callers to import resolveKeysWithKeylessFallback
and KeylessResult directly from the module that defines them (the
resolveKeysWithKeylessFallback module) or add a dedicated non-barrel entrypoint
if a single export surface is required, ensuring you reference the
resolveKeysWithKeylessFallback function and KeylessResult type by their original
module path rather than via this index barrel.
| export { resolveKeysWithKeylessFallback } from './resolveKeysWithKeylessFallback'; | ||
| export type { KeylessResult } from './resolveKeysWithKeylessFallback'; |
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.
Avoid adding new exports in this index.ts barrel.
This extends a barrel file, which is disallowed and can introduce circular dependencies. Expose these via direct module imports or a non‑barrel entrypoint instead of re‑exporting here.
🛠️ Minimal fix (remove the new re-exports)
-export { resolveKeysWithKeylessFallback } from './resolveKeysWithKeylessFallback';
-export type { KeylessResult } from './resolveKeysWithKeylessFallback';As per coding guidelines, “Avoid barrel files (index.ts re-exports) as they can cause circular dependencies”.
📝 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.
| export { resolveKeysWithKeylessFallback } from './resolveKeysWithKeylessFallback'; | |
| export type { KeylessResult } from './resolveKeysWithKeylessFallback'; | |
| export { resolveKeysWithKeylessFallback } from './resolveKeysWithKeylessFallback'; | |
| export type { KeylessResult } from './resolveKeysWithKeylessFallback'; |
🤖 Prompt for AI Agents
In `@packages/shared/src/keyless/index.ts` around lines 15 - 16, Remove the new
barrel re-exports: delete the lines exporting resolveKeysWithKeylessFallback and
the KeylessResult type from the index.ts barrel; instead update callers to
import resolveKeysWithKeylessFallback and KeylessResult directly from the module
that defines them (the resolveKeysWithKeylessFallback module) or add a dedicated
non-barrel entrypoint if a single export surface is required, ensuring you
reference the resolveKeysWithKeylessFallback function and KeylessResult type by
their original module path rather than via this index barrel.
Description
Brings keyless mode support to
@clerk/react-router, removing the need for API keys during development setup.Changes
resolveKeysWithKeylessFallback.ts). SDKs will now write a 10-line wrapper to resolve Clerk keys, falling back to keyless mode in development if configured keys are missing.keylessHelpers.ts)Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Tests