Uh oh!
There was an error while loading. Please reload this page.
feat(api): add clerk api --fapi for the public Frontend API - #345
Conversation
🦋 Changeset detectedLatest commit: efa6d41 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 |
📝 WalkthroughWalkthroughA new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
wyattjoh
left a comment
There was a problem hiding this comment.
Clean, well-scoped addition that mirrors the existing BAPI/PLAPI passthrough patterns. A few follow-ups around dry-run semantics, a duplicated version constant, and silently-ignored flags.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
clerk api spoke only BAPI and PLAPI, so verifying a config change against the instance's public FAPI /v1/environment (what clerk-js consumes) meant dropping to curl and decoding the FAPI domain out of the publishable key by hand. Add --fapi: resolve the FAPI host from the instance's publishable key (via --app/--instance or the linked project) and do an unauthenticated passthrough, reusing the existing lib/fapi.ts client. --fapi and --platform are mutually exclusive. Closes#332
- Move dry-run check before resolveFapiHost so --fapi --dry-run avoids the Platform API round-trip; shows <fapi-host> placeholder instead - Import CLERK_JS_API_VERSION from lib/fapi.ts instead of redeclaring it - Warn when --secret-key is provided with --fapi (key is ignored) - Add tests: --fapi no-app NOT_LINKED error, /environment and /v1/environment path normalization, --secret-key warning, --dry-run no network call
8f9d521 to
64f6f75CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
I think we should just be clearer that this only hits public endpoints.
Because it is possible to use the command line to hit FAPI with curl, and use auth, you just need to use a cookie jar. This is how I do it in my local skill for testing:
FAPI=""
jar=$(mktemp)# 1. Create a dev browser; capture its token.
tok=$(curl -s -c "$jar" -X POST "$FAPI/v1/dev_browser" -H "Origin: $FAPI"| jq -r '.token')# 2. Inspect what the instance has enabled (strategies, factors).
curl -s -b "$jar" -c "$jar""$FAPI/v1/environment?__clerk_db_jwt=$tok" -H "Origin: $FAPI" \
| jq '.auth_config | {first_factors, identification_strategies, password}'# 3. The client state (sessions, sign_in, sign_up).
curl -s -b "$jar" -c "$jar""$FAPI/v1/client?__clerk_db_jwt=$tok" -H "Origin: $FAPI" \
| jq '.response | {sessions: (.sessions|length)}'- Refactor fallback branch of resolveInstance to use resolveFetchedApplicationInstance instead of hand-rolling app.instances.find (addresses wyattjoh comment 3) - Bump CLERK_JS_API_VERSION from "5" to "6" to match current clerk-js major (addresses dmoerner comment 6) - Clarify --fapi help text: "unauthenticated endpoints only" instead of "no auth" to avoid implying it skips auth on authenticated endpoints (addresses dmoerner comment 7)
Uh oh!
There was an error while loading. Please reload this page.
Co-locate the FAPI passthrough request with the other FAPI helpers (bootstrapDevBrowser, fetchUserSettings) in lib/fapi.ts, where it already reached for decodePublishableKey and CLERK_JS_API_VERSION. Promote the passthrough response shape to a shared `ApiResponse` type in lib/fetch.ts so the moved function does not have to import upward from commands/. commands/api/fapi.ts keeps the command-layer instance and host resolution. Addresses review feedback on #345. Claude-Session: https://claude.ai/code/session_01QnfBw9qY7u19BvUWyfQGC6
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli-core/src/lib/fapi.ts`:
- Around line 137-140: The normalizeBapiPath function in the URL construction at
line 137 doesn't correctly handle paths that already contain query strings. When
options.path contains a query string like /v1?foo=bar, normalizeBapiPath can
double-normalize it incorrectly. Separate the path and query string components
of options.path before normalizing, apply normalizeBapiPath only to the path
portion, and then properly reconstruct the URL using the URL constructor so that
both the normalized path and query parameters are handled correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a82c2ba-4088-4f74-b89d-9039b59ff605
📒 Files selected for processing (5)
packages/cli-core/src/commands/api/bapi.tspackages/cli-core/src/commands/api/fapi.tspackages/cli-core/src/commands/api/index.tspackages/cli-core/src/lib/fapi.tspackages/cli-core/src/lib/fetch.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli-core/src/commands/api/index.ts
| const url = new URL(`https://${options.fapiHost}${normalizeBapiPath(options.path)}`); | ||
| if (!url.searchParams.has("_clerk_js_version")) { | ||
| url.searchParams.set("_clerk_js_version", CLERK_JS_API_VERSION); | ||
| } |
There was a problem hiding this comment.
Handle /v1 paths with query strings correctly.
At Line 137, using normalizeBapiPath() can mis-normalize /v1?foo=bar into /v1/v1?foo=bar because the helper only treats / or end-of-string as valid after v1.
Suggested fix (in packages/cli-core/src/lib/bapi-command.ts)
- if (!/^\/v1(?:\/|$)/.test(normalized)) normalized = `/v1${normalized}`;+ if (!/^\/v1(?:\/|$|\?)/.test(normalized)) normalized = `/v1${normalized}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli-core/src/lib/fapi.ts` around lines 137 - 140, The
normalizeBapiPath function in the URL construction at line 137 doesn't correctly
handle paths that already contain query strings. When options.path contains a
query string like /v1?foo=bar, normalizeBapiPath can double-normalize it
incorrectly. Separate the path and query string components of options.path
before normalizing, apply normalizeBapiPath only to the path portion, and then
properly reconstruct the URL using the URL constructor so that both the
normalized path and query parameters are handled correctly.
Summary
clerk apispoke only BAPI (default) and PLAPI (--platform). After a config change, the natural way to verify it took effect is the instance's public FAPI/v1/environmentpayload (what clerk-js actually consumes) — but that meant dropping tocurlplus decoding the FAPI domain out of the publishable key by hand.This adds
--fapi:--app/--instanceor the linked project through the Platform API), reusing the existinglib/fapi.tsclient anddecodePublishableKey./v1-normalized like the other modes, so both/environmentand/v1/environmentwork.--fapiand--platformare mutually exclusive.Implementation reuses the request-target resolution:
api()now builds a{ baseUrl, runRequest }pair, which also de-duplicates the BAPI/PLAPI branches. The local error handler was broadened fromBapiErrortoApiErrorso FAPI error bodies still print to stdout for piping.Test plan
src/commands/api/index.test.ts: host resolution + no auth header;--fapi/--platformconflict; FAPI error body printed to stdout with exit 1Closes#332