Conversation
* feat: add createApiClient factory with typed HTTP methods wrapping axios * feat: add composable interceptor system and built-in auth token injector to ApiClient * feat: add ApiError class and configurable retry with exponential backoff * Revert "feat: add ApiError class and configurable retry with exponential backoff" This reverts commit 2d7c8f3. * Reapply "feat: add ApiError class and configurable retry with exponential backoff" This reverts commit db3568d. * feat: add ApiError class and configurable retry with exponential backoff * test: add comprehensive test suite for ApiClient with ApiError, retry backoff, and interceptor coverag * docs: add README with full API documentation and changeset for v0.1.0
There was a problem hiding this comment.
Pull request overview
This PR repurposes the package into @ciscode/api-kit by introducing a typed Axios-backed API client (with interceptors, retries, and normalized errors) and updating repository metadata and CI/release workflows accordingly.
Changes:
- Added a new
src/apimodule exposingcreateApiClientandApiError, plus comprehensive Vitest coverage. - Updated package metadata (name/version/description/repo) and documented usage in
README.md. - Adjusted CI/release workflows (PR validation on
develop, release checks onmaster, publishing changes) and added Dependabot/CODEOWNERS/changeset.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/index.ts | Exposes the new api entrypoint through the public package surface. |
| src/api/index.ts | Barrel exports for API kit runtime + types. |
| src/api/createApiClient.types.ts | Defines the public TypeScript types for the API client and interceptors. |
| src/api/createApiClient.ts | Implements Axios wrapper with interceptors, retry/backoff, and error normalization. |
| src/api/createApiClient.test.ts | Adds Vitest coverage for client methods, interceptors, errors, and retry logic. |
| src/api/ApiError.ts | Introduces a normalized error type for HTTP/network failures. |
| src/api/ApiError.test.ts | Unit tests for ApiError. |
| package.json | Renames/reversions the package and adds axios dependency + repository metadata. |
| package-lock.json | Updates lockfile to include Axios dependency (but currently inconsistent with package metadata). |
| README.md | Documents the new package purpose and API usage. |
| .github/workflows/release-check.yml | Updates CI for release checks + SonarCloud scan configuration. |
| .github/workflows/publish.yml | Updates publish validation and npm publish options (--provenance). |
| .github/workflows/pr-validation.yml | Adds PR validation workflow for develop. |
| .github/dependabot.yml | Adds monthly dependency update automation. |
| .github/CODEOWNERS | Adds default code ownership. |
| .changeset/release-v0.1.0.md | Adds initial release changeset for @ciscode/api-kit. |
| instance.interceptors.request.use((axiosConfig) => { | ||
| let intercepted: InterceptedRequest = { | ||
| url: axiosConfig.url ?? '', | ||
| method: (axiosConfig.method ?? 'get').toLowerCase(), | ||
| headers: (axiosConfig.headers as unknown as Record<string, string>) ?? {}, | ||
| params: axiosConfig.params as Record<string, unknown> | undefined, | ||
| data: axiosConfig.data as unknown, | ||
| }; |
There was a problem hiding this comment.
InterceptedRequest.headers is typed as Record<string, string>, but it’s initialized by casting axiosConfig.headers (Axios’ internal header structure) to that shape. This is an unsafe cast and can hide non-string header values / non-header properties from Axios’ header object. Consider normalizing headers into a plain Record<string, string> (e.g., via a toJSON()-style conversion when available) before passing to interceptors, so interceptor input matches the declared type.
src/api/createApiClient.types.ts
Outdated
|
|
||
| export type RequestInterceptor = (request: InterceptedRequest) => InterceptedRequest; | ||
| export type ResponseInterceptor = (response: InterceptedResponse) => InterceptedResponse; | ||
| export type ErrorInterceptor = (error: Error) => void; |
There was a problem hiding this comment.
ErrorInterceptor is typed as (error: Error) => void, but createApiClient always passes an ApiError instance. Typing this as (error: ApiError) => void (or a union that includes ApiError) would better reflect actual behavior and let consumers safely access status, code, and details without needing a manual type guard.
| export type ErrorInterceptor = (error: Error) => void; | |
| export type ErrorInterceptor = (error: ApiError) => void; |
| it('should apply exponential backoff: retryDelay * 2^attempt', async () => { | ||
| const delays: number[] = []; | ||
| const originalSetTimeout = globalThis.setTimeout; | ||
| vi.stubGlobal('setTimeout', (fn: () => void, ms?: number) => { | ||
| if (ms !== undefined && ms > 0) delays.push(ms); | ||
| return originalSetTimeout(fn, 0); | ||
| }); | ||
|
|
||
| const retryClient = createApiClient({ | ||
| baseURL: 'https://api.example.com', | ||
| retry: 3, | ||
| retryDelay: 500, | ||
| }); | ||
|
|
||
| let callCount = 0; | ||
| mockState.instance.get = vi.fn(() => { | ||
| callCount++; | ||
| if (callCount <= 3) { | ||
| return Promise.reject( | ||
| createAxiosError('Service Unavailable', { status: 503, data: null }), | ||
| ); | ||
| } | ||
| return Promise.resolve({ status: 200, headers: {}, data: { ok: true } }); | ||
| }); | ||
|
|
||
| const result = await retryClient.get('/backoff'); | ||
| expect(result).toEqual({ ok: true }); | ||
| expect(delays).toEqual([500, 1000, 2000]); | ||
|
|
||
| vi.unstubAllGlobals(); | ||
| }); |
There was a problem hiding this comment.
These tests stub globalThis.setTimeout and only call vi.unstubAllGlobals() at the end of the test body. If an assertion fails before that line, the stub can leak into subsequent tests and cause cascading failures. Use afterEach(() => vi.unstubAllGlobals()) or a try/finally to guarantee cleanup.
.github/workflows/release-check.yml
Outdated
| -Dsonar.organization=${{ env.SONAR_ORGANIZATION }} | ||
| -Dsonar.projectKey=${{ env.SONAR_PROJECT_KEY }} | ||
| -Dsonar.sources=src | ||
| -Dsonar.tests=test |
There was a problem hiding this comment.
The SonarCloud configuration sets -Dsonar.tests=test, but this repo’s tests are under src/ (e.g., src/api/*.test.ts, src/__tests__/). With the current setting, Sonar will likely miss tests and report incorrect coverage. Update sonar.tests (and/or sonar.test.inclusions) to match the actual test locations.
| -Dsonar.tests=test | |
| -Dsonar.tests=src | |
| -Dsonar.test.inclusions=src/**/*.test.ts,src/**/*.test.tsx,src/**/*.spec.ts,src/**/*.spec.tsx,src/**/__tests__/** |
| - name: SonarCloud Scan | ||
| if: ${{ github.event_name == 'workflow_dispatch' && github.event.inputs.sonar == 'true' }} | ||
| uses: SonarSource/sonarqube-scan-action@v6 | ||
| env: | ||
| SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} | ||
| SONAR_HOST_URL: ${{ env.SONAR_HOST_URL }} |
There was a problem hiding this comment.
This workflow always runs the SonarCloud scan on PRs, but SONAR_TOKEN is taken from repo secrets. For PRs from forks, secrets are not available and the job will fail. Consider gating the Sonar steps to only run when the PR comes from the same repo (non-fork), or otherwise make the scan optional so external contributions don’t break CI.
| - name: Validate version tag and package.json | ||
| run: | | ||
| TAG=$(git describe --exact-match --tags HEAD 2>/dev/null || echo "") | ||
| if [[ -z "$TAG" ]]; then | ||
| echo "❌ No tag found on HEAD. This push did not include a version tag." | ||
| echo "To publish, merge to master with a tag: git tag v1.0.0 && git push origin master --tags" | ||
| PKG_VERSION=$(grep '"version"' package.json | head -1 | sed 's/.*"version": "\([^"]*\)".*/\1/') | ||
| TAG="v${PKG_VERSION}" | ||
|
|
||
| if [[ -z "$PKG_VERSION" ]]; then | ||
| echo "❌ ERROR: Could not read version from package.json" | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [[ ! "$TAG" =~ ^v[0-9]+\.[0-9]+\.[0-9]+$ ]]; then | ||
| echo "❌ Invalid tag format: $TAG. Expected: v*.*.*" | ||
| echo "❌ ERROR: Invalid version format in package.json: '$PKG_VERSION'" | ||
| echo "Expected format: x.y.z (e.g., 1.0.0, 0.2.3)" | ||
| exit 1 | ||
| fi | ||
|
|
||
| if ! git rev-parse "$TAG" >/dev/null 2>&1; then | ||
| echo "❌ ERROR: Tag $TAG not found!" | ||
| echo "" | ||
| echo "This typically happens when:" | ||
| echo " 1. You forgot to run 'npm version patch|minor|major' on your feature branch" | ||
| echo " 2. You didn't push the tag: git push origin <feat/your-feature> --tags" | ||
| echo " 3. The tag was created locally but never pushed to remote" | ||
| echo "" | ||
| echo "📋 Correct workflow:" | ||
| echo " 1. On feat/** or feature/**: npm version patch (or minor/major)" | ||
| echo " 2. Push branch + tag: git push origin feat/your-feature --tags" | ||
| echo " 3. PR feat/** → develop, then PR develop → master" | ||
| echo " 4. Workflow automatically triggers on master push" | ||
| echo "" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
The publish workflow now only checks that the computed version tag exists somewhere in the repo, not that it points to the commit being built. This can allow publishing from an untagged HEAD (or a HEAD whose package.json version matches an older tag). Validate that TAG resolves to HEAD (or that HEAD is exactly tagged) before proceeding.
src/api/createApiClient.ts
Outdated
| function toInterceptedResponse(axiosRes: { | ||
| status: number; | ||
| headers: unknown; | ||
| data: unknown; | ||
| }): InterceptedResponse { | ||
| const raw = axiosRes.headers as Record<string, string> | undefined; | ||
| return { | ||
| status: axiosRes.status, | ||
| headers: raw ?? {}, |
There was a problem hiding this comment.
toInterceptedResponse casts axiosRes.headers to Record<string, string>, but Axios response headers commonly contain non-string values (e.g., string[]) and can be an AxiosHeaders object. This cast makes interceptor/header typing inaccurate. Consider exposing headers as Record<string, string | string[]> (or Record<string, unknown>) and/or normalizing header values to strings before returning them.
| function toInterceptedResponse(axiosRes: { | |
| status: number; | |
| headers: unknown; | |
| data: unknown; | |
| }): InterceptedResponse { | |
| const raw = axiosRes.headers as Record<string, string> | undefined; | |
| return { | |
| status: axiosRes.status, | |
| headers: raw ?? {}, | |
| function normalizeResponseHeaders(headers: unknown): Record<string, string> { | |
| if (!headers || typeof headers !== 'object') { | |
| return {}; | |
| } | |
| const headersSource = | |
| 'toJSON' in headers && typeof headers.toJSON === 'function' | |
| ? headers.toJSON() | |
| : headers; | |
| if (!headersSource || typeof headersSource !== 'object') { | |
| return {}; | |
| } | |
| const normalized: Record<string, string> = {}; | |
| for (const [key, value] of Object.entries(headersSource)) { | |
| if (Array.isArray(value)) { | |
| normalized[key] = value.map((item) => String(item)).join(', '); | |
| continue; | |
| } | |
| if (value != null) { | |
| normalized[key] = String(value); | |
| } | |
| } | |
| return normalized; | |
| } | |
| function toInterceptedResponse(axiosRes: { | |
| status: number; | |
| headers: unknown; | |
| data: unknown; | |
| }): InterceptedResponse { | |
| return { | |
| status: axiosRes.status, | |
| headers: normalizeResponseHeaders(axiosRes.headers), |
| for (const interceptor of responseInterceptors) { | ||
| intercepted = interceptor(intercepted); | ||
| } | ||
|
|
There was a problem hiding this comment.
ResponseInterceptor can return a modified InterceptedResponse (status/headers/data), but the implementation only applies intercepted.data back onto axiosResponse. This makes updates to status/headers silently ignored and the type contract misleading. Either restrict the interceptor type to only transform data, or also apply status/headers changes back to the underlying response (as feasible) so interceptor behavior matches the type.
| axiosResponse.status = intercepted.status; | |
| axiosResponse.headers = | |
| intercepted.headers as typeof axiosResponse.headers; |
| describe('getToken auth interceptor', () => { | ||
| it('should inject Authorization header when getToken returns a value', async () => { | ||
| const tokenClient = createApiClient({ | ||
| baseURL: 'https://api.example.com', | ||
| getToken: () => 'my-token-123', | ||
| }); | ||
| mockState.setResponse('get', '/secure', { data: 'secret' }); | ||
|
|
||
| await tokenClient.get('/secure'); | ||
|
|
||
| const requestCall = mockState.instance.get.mock.calls[0]; | ||
| expect(requestCall).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
The "should inject Authorization header" test doesn't actually assert that the Authorization header was added (it only checks that a request call exists). This can pass even if token injection is broken. Add an assertion that the outgoing request config includes the expected Authorization: Bearer ... header.
| export * from './components'; | ||
| export * from './hooks'; | ||
| export * from './utils'; |
There was a problem hiding this comment.
src/index.ts now exports the new api surface, but it still re-exports placeholder components, hooks, and utils modules (e.g., __components_placeholder). For an API client package, this expands the public surface with unrelated exports and can confuse consumers. Consider removing these placeholder exports (or updating them to real, documented APIs) so the public API matches the package’s stated purpose.
| export * from './components'; | |
| export * from './hooks'; | |
| export * from './utils'; |
|
Summary
Why
Checklist
npm run lintpassesnpm run typecheckpassesnpm testpassesnpm run buildpassesnpx changeset) if this affects consumersNotes