fix(backend): Support ExpiresInSeconds param #6150
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 85d17ca The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 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 |
jacekradko
force-pushed
the
fix/add-expire-in-seconds-param
branch
from
20f89f0 to
0425848
Compare
📝 WalkthroughWalkthroughThis change introduces support for an optional Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. All modifications are directly related to the implementation, typing, and testing of the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/backend/src/tokens/authObjects.ts (1)
424-433:createGetTokenignoresexpiresInSecondswhen no template is givenIf the caller passes
{ expiresInSeconds: 3600 }without a template, the current guard (if (options.template)) short-circuits, returning the rawsessionTokenand never hitting the backend. This contradicts the newSessionAPIbehaviour and the test case “creates a session token without template and with expiresInSeconds”.Proposed fix:
- if (options.template) { - return fetcher(sessionId, options.template, options.expiresInSeconds); - } - - return sessionToken; + const needsRemoteToken = + options.template !== undefined || + options.expiresInSeconds !== undefined; + + if (needsRemoteToken) { + return fetcher(sessionId, options.template, options.expiresInSeconds); + } + + return sessionToken;
🧹 Nitpick comments (4)
.changeset/evil-paws-learn.md (1)
6-7: Consider clarifying the changeset summaryA tiny wording tweak will read more naturally and match the tense of other changesets:
-Add Support for expiresInSeconds body param +Add support for the `expiresInSeconds` body parampackages/backend/src/api/endpoints/SessionApi.ts (1)
94-103: AvoidanyinrequestOptions
requestOptionsis typed asany, defeating TypeScript’s safety. You can keep strictness by re-using the existingRequestOptionstype thatAbstractAPI.requestexpects (or declare a minimal interface withmethod,path,bodyParams?).- const requestOptions: any = { + const requestOptions: { + method: 'POST'; + path: string; + bodyParams?: { expires_in_seconds: number }; + } = { method: 'POST', path, };packages/backend/src/tokens/__tests__/authObjects.test.ts (2)
17-20: Improve typing for the mockedcreateBackendApiClient
vi.mock('../../api/factory', () => ({ createBackendApiClient: vi.fn() }));hides the true call-signature ofcreateBackendApiClient, so IDE/autocomplete assistance and compile-time checks are lost. Prefer re-exporting the original type when stubbing:-vi.mock('../../api/factory', () => ({ - createBackendApiClient: vi.fn(), -})); +vi.mock('../../api/factory', async () => { + const actual = await vi.importActual<typeof import('../../api/factory')>('../../api/factory'); + + return { + ...actual, + createBackendApiClient: vi.fn<Parameters<typeof actual.createBackendApiClient>, ReturnType<typeof actual.createBackendApiClient>>(), + }; +});That way the mock keeps the same parameter / return-type shape, reducing the chance of silent regressions when the real function’s signature changes.
441-516: Consolidate repeated mock setup & ensure isolationEach
itblock duplicates themockGetToken/mockApiClientboot-strap and overridescreateBackendApiClient. Consider extracting this into abeforeEachand resetting mocks in anafterEachto keep the tests focused on assertions and avoid accidental state leakage:-describe('getToken with expiresInSeconds support', () => { - it('calls fetcher with expiresInSeconds when template is provided', async () => { - const mockGetToken = vi.fn().mockResolvedValue({ jwt: 'mocked-jwt-token' }); - ... - }); - ... -}); +describe('getToken with expiresInSeconds support', () => { + let mockGetToken: ReturnType<typeof vi.fn>; + + beforeEach(() => { + mockGetToken = vi.fn().mockResolvedValue({ jwt: 'mocked-jwt-token' }); + vi.mocked(createBackendApiClient).mockReturnValue({ + sessions: { getToken: mockGetToken }, + } as any); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it('calls fetcher with expiresInSeconds when template is provided', async () => { + ... + }); + ... +});Benefits:
- Less duplication – easier to update test fixtures in one place.
- Guarantees mocks are reset between cases, preventing false positives/negatives if future tests mutate
mockGetTokenbehaviour.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/evil-paws-learn.md(1 hunks)packages/backend/src/api/__tests__/SessionApi.test.ts(1 hunks)packages/backend/src/api/endpoints/SessionApi.ts(1 hunks)packages/backend/src/tokens/__tests__/authObjects.test.ts(3 hunks)packages/backend/src/tokens/authObjects.ts(2 hunks)packages/types/src/ssr.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
packages/types/src/ssr.ts (1)
18-24: Runtime validation is still advisableThe JSDoc states
expiresInSeconds“Must be a positive integer”, but nothing enforces this. Consider adding a runtime guard where the option is first consumed (e.g. inSessionAPI.getToken) to throw early for invalid values.packages/backend/src/api/__tests__/SessionApi.test.ts (1)
53-68: Great coverageNice job adding the edge-case test for “no template but custom expiry”; this will catch the logic gap flagged in
createGetToken. 💯packages/backend/src/tokens/__tests__/authObjects.test.ts (1)
492-515: Missing assertion: verify backend client is not invoked whentemplateis absentThe third test checks that
sessions.getTokenis untouched, but it would also be valuable to assert thatcreateBackendApiClientitself is never called, ensuring the short-circuit logic really skips any network work:expect(mockGetToken).not.toHaveBeenCalled(); +expect(createBackendApiClient).not.toHaveBeenCalled(); expect(result).toBe('raw-session-token');This catches accidental regressions where the implementation starts instantiating the API client even when unnecessary.
| type TokenFetcher = (sessionId: string, template: string, expiresInSeconds?: number) => Promise<string>; | ||
|
|
||
| /** | ||
| * Factory function type that creates a getToken function for auth objects. | ||
| * | ||
| * @param params - Configuration object containing session information and token fetcher | ||
| * @returns A ServerGetToken function that can be used to retrieve tokens | ||
| */ | ||
| type CreateGetToken = (params: { sessionId: string; sessionToken: string; fetcher: TokenFetcher }) => ServerGetToken; |
There was a problem hiding this comment.
TokenFetcher should allow omitted template
TokenFetcher currently requires a template: string, but the new API allows sending no template while still supplying expiresInSeconds.
Make template optional to match the actual usage that the tests and SessionAPI permit.
-type TokenFetcher = (sessionId: string, template: string, expiresInSeconds?: number) => Promise<string>;
+type TokenFetcher = (
+ sessionId: string,
+ template?: string, // may be '', undefined or a real template
+ expiresInSeconds?: number,
+) => Promise<string>;📝 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.
| type TokenFetcher = (sessionId: string, template: string, expiresInSeconds?: number) => Promise<string>; | |
| /** | |
| * Factory function type that creates a getToken function for auth objects. | |
| * | |
| * @param params - Configuration object containing session information and token fetcher | |
| * @returns A ServerGetToken function that can be used to retrieve tokens | |
| */ | |
| type CreateGetToken = (params: { sessionId: string; sessionToken: string; fetcher: TokenFetcher }) => ServerGetToken; | |
| type TokenFetcher = ( | |
| sessionId: string, | |
| template?: string, // may be '', undefined or a real template | |
| expiresInSeconds?: number, | |
| ) => Promise<string>; | |
| /** | |
| * Factory function type that creates a getToken function for auth objects. | |
| * | |
| * @param params - Configuration object containing session information and token fetcher | |
| * @returns A ServerGetToken function that can be used to retrieve tokens | |
| */ | |
| type CreateGetToken = (params: { sessionId: string; sessionToken: string; fetcher: TokenFetcher }) => ServerGetToken; |
🤖 Prompt for AI Agents
In packages/backend/src/tokens/authObjects.ts around lines 398 to 406, the
TokenFetcher type currently requires the template parameter as a string, but the
new API allows calling it without a template while still providing
expiresInSeconds. To fix this, update the TokenFetcher type definition to make
the template parameter optional by adding a question mark, so it matches the
actual usage in tests and SessionAPI.
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
Description
Add Support for
expiresInSecondsbody param: https://clerk.com/docs/reference/backend-api/tag/Sessions#operation/CreateSessionTokenFromTemplateFixes: #6141
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit