Feat: contract definition #669
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change set introduces a type-safe, composable framework for defining, configuring, and consuming API contracts and HTTP headers in TypeScript. It adds core modules for contract definition ( Sequence Diagram(s)sequenceDiagram
participant User
participant ContractService
participant ClientResolver
participant HeaderBuilder
participant SendFunction
User->>ContractService: Call service method (e.g., getUser)
ContractService->>ClientResolver: Await client for service
ContractService->>HeaderBuilder: Resolve contract headers
ContractService->>User: Accept user headers
ContractService->>HeaderBuilder: Merge user and contract headers
ContractService->>SendFunction: Dispatch request (GET/DELETE/PAYLOAD) with client, route, headers, params
SendFunction-->>ContractService: Return response
ContractService-->>User: Return response
Suggested reviewers
🪧 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: 12
🔭 Outside diff range comments (1)
packages/app/api-contracts/src/contractService.ts (1)
124-455: Remove large block of legacy/experimental code.This 330+ line block of commented code should be removed from the source file. If this code is needed for reference, it should be moved to documentation or a separate file.
Delete lines 124-455 entirely. If this code contains valuable design decisions or experimental approaches, consider:
- Moving relevant parts to documentation
- Creating a separate design document
- Keeping only the most relevant parts as inline comments
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
packages/app/api-contracts/src/apiContracts.ts(2 hunks)packages/app/api-contracts/src/contractService.ts(1 hunks)packages/app/api-contracts/src/headers/createHeaderBuilderMiddleware.test.ts(1 hunks)packages/app/api-contracts/src/headers/createHeaderBuilderMiddleware.ts(1 hunks)packages/app/api-contracts/src/headers/headerBuilder.test.ts(1 hunks)packages/app/api-contracts/src/headers/headerBuilder.ts(1 hunks)packages/app/frontend-http-client/package.json(1 hunks)packages/app/frontend-http-client/src/contract.ts(1 hunks)packages/app/frontend-http-client/src/temp.ts(1 hunks)packages/app/frontend-http-client/src/types.ts(2 hunks)packages/app/universal-ts-utils/src/node.ts(1 hunks)packages/app/universal-ts-utils/src/public/type/assertIsNever.test.ts(1 hunks)packages/app/universal-ts-utils/src/public/type/assertIsNever.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
packages/app/universal-ts-utils/src/public/type/assertIsNever.test.ts (1)
packages/app/universal-ts-utils/src/public/type/assertIsNever.ts (1)
assertIsNever(1-3)
packages/app/api-contracts/src/headers/createHeaderBuilderMiddleware.test.ts (2)
packages/app/api-contracts/src/headers/createHeaderBuilderMiddleware.ts (1)
createHeaderBuilderMiddleware(10-14)packages/app/api-contracts/src/headers/headerBuilder.ts (2)
HeaderBuilder(41-211)resolve(205-210)
packages/app/api-contracts/src/headers/headerBuilder.test.ts (2)
packages/app/api-contracts/src/headers/headerBuilder.ts (1)
HeaderBuilder(41-211)packages/app/api-contracts/src/headers/createHeaderBuilderMiddleware.ts (1)
createHeaderBuilderMiddleware(10-14)
packages/app/frontend-http-client/src/temp.ts (1)
packages/app/api-contracts/src/headers/headerBuilder.ts (1)
Headers(3-3)
packages/app/api-contracts/src/apiContracts.ts (1)
packages/app/api-contracts/src/contractService.ts (3)
AnyGetRoute(8-8)AnyDeleteRoute(9-9)AnyPayloadRoute(10-10)
packages/app/api-contracts/src/headers/createHeaderBuilderMiddleware.ts (1)
packages/app/api-contracts/src/headers/headerBuilder.ts (2)
Headers(3-3)HeaderBuilder(41-211)
packages/app/api-contracts/src/headers/headerBuilder.ts (1)
packages/app/api-contracts/src/headers/createHeaderBuilderMiddleware.ts (1)
HeaderBuilderMiddleware(19-19)
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (24.x)
- GitHub Check: build (22.x)
- GitHub Check: build (20.x)
🔇 Additional comments (10)
packages/app/universal-ts-utils/src/node.ts (1)
32-32: Export addition follows established patterns.The new export for
assertIsNeveris correctly placed in the type exports section and follows the established pattern of using.jsextension for ESM module resolution.packages/app/frontend-http-client/package.json (2)
44-44: Peer dependency addition is appropriate.The addition of
@lokalise/universal-ts-utilsas a peer dependency with version constraint ">=4.4.1" correctly supports the new contract service functionality that depends on utilities likeassertIsNever.
51-51: Dev dependency addition supports development needs.The dev dependency addition with version constraint "^4.4.1" is appropriate for development and testing purposes.
packages/app/frontend-http-client/src/contract.ts (1)
60-62: assertIsNever correctly enforces exhaustive checksThe
assertIsNeverfunction from@lokalise/universal-ts-utilsdoes exactly what’s needed: it forces TypeScript to error at compile time if any newroutecases aren’t handled in the switch. No further changes are required here.packages/app/frontend-http-client/src/temp.ts (1)
1-13: Clarify intent of this placeholder fileThis file (
packages/app/frontend-http-client/src/temp.ts) currently only declares two empty functions without any logic:
initialiseContractService()createServiceMethod<…>(definition: ContractDefinition<…>)Before moving forward, could you clarify:
- Is this meant to sketch out the planned API surface for feedback?
- Should the implementations be added as part of this PR?
- Or would you prefer to remove this file now and track it in a separate issue?
Let me know if you need help implementing these methods here or if we should park them elsewhere.
packages/app/api-contracts/src/apiContracts.ts (1)
326-328: Good module organization with re-exportsThe re-export structure provides a clean public API surface for the contract system.
packages/app/api-contracts/src/headers/createHeaderBuilderMiddleware.ts (2)
42-59: Well-designed middleware pattern with lazy evaluationExcellent implementation that:
- Preserves type safety with intersection types
- Uses lazy evaluation via
from()method- Handles both sync and async middleware correctly
- Provides clear documentation with examples
21-23: Type constraint mismatch in MiddlewareFnThe
MiddlewareFntype accepts aHeaderBuilder<Headers>parameter but the generic constraint in line 11 uses justHeaderBuilderwithout type parameter, which provides better type inference.Consider aligning the types:
type MiddlewareFn<H extends Headers> = ( - builder: HeaderBuilder<Headers>, + builder: HeaderBuilder, ) => HeaderBuilder<H> | Promise<HeaderBuilder<H>>Likely an incorrect or invalid review comment.
packages/app/frontend-http-client/src/types.ts (1)
290-310: Well-designed type inference for contract services.The
ContractServicetype elegantly maps route definitions to typed service methods, with proper parameter and return type inference based on route kind (GET, DELETE, or payload). This provides excellent type safety for API consumers.packages/app/api-contracts/src/contractService.ts (1)
45-53: Remove or implement placeholder service methodsI’ve spotted several commented-out blocks related to
serviceandserviceMethodsin packages/app/api-contracts/src/contractService.ts. Before merging, please decide whether to fully implement this functionality or remove these placeholders:• Lines 45–53
• Lines 68–74
• Lines 85–94
• Lines 116–120Let me know if you’d like help implementing these service methods or if we should clean them up.
Comment on lines
+1
to
+3
| export const assertIsNever = (value: never): never => { | ||
| throw new Error(`Unexpected value: ${value}`) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Utility function implementation is correct with minor improvement opportunity.
The assertIsNever function correctly implements exhaustive type checking. The implementation follows the expected pattern for never-type assertions.
Consider enhancing the error message handling for better debugging:
-export const assertIsNever = (value: never): never => {
- throw new Error(`Unexpected value: ${value}`)
-}
+export const assertIsNever = (value: never): never => {
+ throw new Error(`Unexpected value: ${JSON.stringify(value)}`)
+}📝 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.
Suggested change
| export const assertIsNever = (value: never): never => { | |
| throw new Error(`Unexpected value: ${value}`) | |
| } | |
| export const assertIsNever = (value: never): never => { | |
| throw new Error(`Unexpected value: ${JSON.stringify(value)}`) | |
| } |
🤖 Prompt for AI Agents
In packages/app/universal-ts-utils/src/public/type/assertIsNever.ts at lines 1
to 3, improve the error message in the assertIsNever function by including more
context or stringifying the unexpected value to make debugging easier. Update
the Error constructor to provide a clearer, more informative message about the
unexpected value encountered.
Comment on lines
+4
to
+8
| describe('assertIsNever', () => { | ||
| it('should throw an error if a non-never value is passed in', () => { | ||
| expect(() => assertIsNever('not-never' as never)).toThrowError() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Test implementation is correct with room for improvement.
The test correctly verifies that assertIsNever throws an error when called with a non-never value. The type casting approach is appropriate for testing runtime behavior.
Consider making the test more specific about the expected error:
- it('should throw an error if a non-never value is passed in', () => {
- expect(() => assertIsNever('not-never' as never)).toThrowError()
- })
+ it('should throw an error if a non-never value is passed in', () => {
+ expect(() => assertIsNever('not-never' as never)).toThrowError('Unexpected value: not-never')
+ })📝 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.
Suggested change
| describe('assertIsNever', () => { | |
| it('should throw an error if a non-never value is passed in', () => { | |
| expect(() => assertIsNever('not-never' as never)).toThrowError() | |
| }) | |
| }) | |
| describe('assertIsNever', () => { | |
| it('should throw an error if a non-never value is passed in', () => { | |
| expect(() => assertIsNever('not-never' as never)).toThrowError('Unexpected value: not-never') | |
| }) | |
| }) |
🤖 Prompt for AI Agents
In packages/app/universal-ts-utils/src/public/type/assertIsNever.test.ts between
lines 4 and 8, the test correctly checks that assertIsNever throws an error for
a non-never value but lacks specificity about the error. Update the test to
expect a specific error message or error type in the toThrowError assertion to
make the test more precise and informative.
Comment on lines
+1
to
+134
| import { createHeaderBuilderMiddleware } from './createHeaderBuilderMiddleware.js' | ||
| import { HeaderBuilder } from './headerBuilder.js' | ||
| import { describe, expect, vitest } from 'vitest' | ||
|
|
||
| // biome-ignore lint/complexity/noBannedTypes: To test empty headers, we need to use an empty object type. | ||
| type ExpectedEmptyHeaders = {} | ||
|
|
||
| // It is critical that type safety is maintained when refactoring - this offers a way to unit test the type-system. | ||
| // The call will fail to build if the expected type is not correct. | ||
| const typeCheck = <const Expected>(_actual: Expected): void => {} | ||
|
|
||
| describe('HeaderBuilder', () => { | ||
| it('should create an empty header builder', async () => { | ||
| const builder = HeaderBuilder.create() | ||
|
|
||
| const actual = await builder.resolve() | ||
|
|
||
| typeCheck<ExpectedEmptyHeaders>(actual) | ||
|
|
||
| expect(actual).toEqual({}) | ||
| }) | ||
|
|
||
| it('should create a header builder with default headers', async () => { | ||
| const builder = HeaderBuilder.create({ 'Content-Type': 'application/json' }) | ||
|
|
||
| const actual = await builder.resolve() | ||
|
|
||
| typeCheck<{ 'Content-Type': 'application/json' }>(actual) | ||
|
|
||
| expect(actual).toEqual({ 'Content-Type': 'application/json' }) | ||
| }) | ||
|
|
||
| it('should allow individual headers to be added to the builder by name', async () => { | ||
| const builder = HeaderBuilder.create().add('Content-Type', 'application/json') | ||
|
|
||
| const actual = await builder.resolve() | ||
|
|
||
| typeCheck<{ 'Content-Type': 'application/json' }>(actual) | ||
|
|
||
| expect(actual).toEqual({ 'Content-Type': 'application/json' }) | ||
| }) | ||
|
|
||
| it('should allow multiple headers to be added in one static object', async () => { | ||
| const builder = HeaderBuilder.create().and({ | ||
| 'Content-Type': 'application/json', | ||
| 'X-Api-Key': '1234', | ||
| }) | ||
|
|
||
| const actual = await builder.resolve() | ||
|
|
||
| typeCheck<{ 'Content-Type': 'application/json'; 'X-Api-Key': '1234' }>(actual) | ||
|
|
||
| expect(actual).toEqual({ 'Content-Type': 'application/json', 'X-Api-Key': '1234' }) | ||
| }) | ||
|
|
||
| it('should allow multiple headers to be added in a promise of one static object', async () => { | ||
| const builder = HeaderBuilder.create().and( | ||
| Promise.resolve({ | ||
| 'Content-Type': 'application/json', | ||
| 'X-Api-Key': '1234', | ||
| }), | ||
| ) | ||
|
|
||
| const actual = await builder.resolve() | ||
|
|
||
| typeCheck<{ 'Content-Type': 'application/json'; 'X-Api-Key': '1234' }>(actual) | ||
|
|
||
| expect(actual).toEqual({ 'Content-Type': 'application/json', 'X-Api-Key': '1234' }) | ||
| }) | ||
|
|
||
| it('should allow headers to be added from a factory function', async () => { | ||
| const builder = HeaderBuilder.create().from(async () => { | ||
| const token = await mockGetToken() | ||
| return { authorization: `Bearer ${token}` } | ||
| }) | ||
|
|
||
| const actual = await builder.resolve() | ||
|
|
||
| typeCheck<{ authorization: string }>(actual) | ||
|
|
||
| expect(actual).toEqual({ authorization: 'Bearer 1234' }) | ||
| }) | ||
|
|
||
| const mockGetToken = async () => '1234' | ||
|
|
||
| it('should allow you to extend the builder with middleware functions', async () => { | ||
| const middleware = createHeaderBuilderMiddleware(async (builder) => { | ||
| const token = await mockGetToken() | ||
| return builder.and({ authorization: `Bearer ${token}` }) | ||
| }) | ||
|
|
||
| const builder = HeaderBuilder.create().with(middleware) | ||
|
|
||
| const actual = await builder.resolve() | ||
|
|
||
| typeCheck<{ authorization: string }>(actual) | ||
|
|
||
| expect(actual).toEqual({ authorization: 'Bearer 1234' }) | ||
| }) | ||
|
|
||
| it('should lazy load promises and only resolve them when the headers are resolved', async () => { | ||
| const factory = vitest.fn(async () => ({ 'Content-Type': 'application/json' }) as const) | ||
| const middlewareMock = vitest.fn(async (builder: HeaderBuilder) => | ||
| builder.add('X-Api-Key', '1234'), | ||
| ) | ||
|
|
||
| const middleware = createHeaderBuilderMiddleware(middlewareMock) | ||
|
|
||
| const builder = HeaderBuilder.create().from(factory).with(middleware) | ||
|
|
||
| expect(factory).not.toHaveBeenCalled() | ||
| expect(middlewareMock).not.toHaveBeenCalled() | ||
|
|
||
| const actual = await builder.resolve() | ||
|
|
||
| typeCheck<{ 'Content-Type': 'application/json'; 'X-Api-Key': '1234' }>(actual) | ||
|
|
||
| expect(factory).toHaveBeenCalled() | ||
| expect(actual).toEqual({ 'Content-Type': 'application/json', 'X-Api-Key': '1234' }) | ||
| }) | ||
|
|
||
| it('should merge two header builders', async () => { | ||
| const builder1 = HeaderBuilder.create().add('Content-Type', 'application/json') | ||
| const builder2 = HeaderBuilder.create().add('authorization', 'Bearer token') | ||
|
|
||
| const mergedBuilder = builder1.merge(builder2) | ||
|
|
||
| const actual = await mergedBuilder.resolve() | ||
|
|
||
| typeCheck<{ 'Content-Type': 'application/json'; authorization: 'Bearer token' }>(actual) | ||
|
|
||
| expect(actual).toEqual({ 'Content-Type': 'application/json', authorization: 'Bearer token' }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Comprehensive test suite with excellent coverage.
The test suite thoroughly covers the HeaderBuilder functionality with appropriate type safety verification. The tests are well-structured and cover both synchronous and asynchronous operations, lazy evaluation, middleware integration, and builder merging.
The typeCheck function is an excellent approach for ensuring type safety is maintained during refactoring.
Minor organizational improvement:
+const mockGetToken = async () => '1234'
+
describe('HeaderBuilder', () => {
it('should create an empty header builder', async () => {
const builder = HeaderBuilder.create()
@@ -81,8 +82,6 @@
expect(actual).toEqual({ authorization: 'Bearer 1234' })
})
- const mockGetToken = async () => '1234'
-
it('should allow you to extend the builder with middleware functions', async () => {📝 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.
Suggested change
| import { createHeaderBuilderMiddleware } from './createHeaderBuilderMiddleware.js' | |
| import { HeaderBuilder } from './headerBuilder.js' | |
| import { describe, expect, vitest } from 'vitest' | |
| // biome-ignore lint/complexity/noBannedTypes: To test empty headers, we need to use an empty object type. | |
| type ExpectedEmptyHeaders = {} | |
| // It is critical that type safety is maintained when refactoring - this offers a way to unit test the type-system. | |
| // The call will fail to build if the expected type is not correct. | |
| const typeCheck = <const Expected>(_actual: Expected): void => {} | |
| describe('HeaderBuilder', () => { | |
| it('should create an empty header builder', async () => { | |
| const builder = HeaderBuilder.create() | |
| const actual = await builder.resolve() | |
| typeCheck<ExpectedEmptyHeaders>(actual) | |
| expect(actual).toEqual({}) | |
| }) | |
| it('should create a header builder with default headers', async () => { | |
| const builder = HeaderBuilder.create({ 'Content-Type': 'application/json' }) | |
| const actual = await builder.resolve() | |
| typeCheck<{ 'Content-Type': 'application/json' }>(actual) | |
| expect(actual).toEqual({ 'Content-Type': 'application/json' }) | |
| }) | |
| it('should allow individual headers to be added to the builder by name', async () => { | |
| const builder = HeaderBuilder.create().add('Content-Type', 'application/json') | |
| const actual = await builder.resolve() | |
| typeCheck<{ 'Content-Type': 'application/json' }>(actual) | |
| expect(actual).toEqual({ 'Content-Type': 'application/json' }) | |
| }) | |
| it('should allow multiple headers to be added in one static object', async () => { | |
| const builder = HeaderBuilder.create().and({ | |
| 'Content-Type': 'application/json', | |
| 'X-Api-Key': '1234', | |
| }) | |
| const actual = await builder.resolve() | |
| typeCheck<{ 'Content-Type': 'application/json'; 'X-Api-Key': '1234' }>(actual) | |
| expect(actual).toEqual({ 'Content-Type': 'application/json', 'X-Api-Key': '1234' }) | |
| }) | |
| it('should allow multiple headers to be added in a promise of one static object', async () => { | |
| const builder = HeaderBuilder.create().and( | |
| Promise.resolve({ | |
| 'Content-Type': 'application/json', | |
| 'X-Api-Key': '1234', | |
| }), | |
| ) | |
| const actual = await builder.resolve() | |
| typeCheck<{ 'Content-Type': 'application/json'; 'X-Api-Key': '1234' }>(actual) | |
| expect(actual).toEqual({ 'Content-Type': 'application/json', 'X-Api-Key': '1234' }) | |
| }) | |
| it('should allow headers to be added from a factory function', async () => { | |
| const builder = HeaderBuilder.create().from(async () => { | |
| const token = await mockGetToken() | |
| return { authorization: `Bearer ${token}` } | |
| }) | |
| const actual = await builder.resolve() | |
| typeCheck<{ authorization: string }>(actual) | |
| expect(actual).toEqual({ authorization: 'Bearer 1234' }) | |
| }) | |
| const mockGetToken = async () => '1234' | |
| it('should allow you to extend the builder with middleware functions', async () => { | |
| const middleware = createHeaderBuilderMiddleware(async (builder) => { | |
| const token = await mockGetToken() | |
| return builder.and({ authorization: `Bearer ${token}` }) | |
| }) | |
| const builder = HeaderBuilder.create().with(middleware) | |
| const actual = await builder.resolve() | |
| typeCheck<{ authorization: string }>(actual) | |
| expect(actual).toEqual({ authorization: 'Bearer 1234' }) | |
| }) | |
| it('should lazy load promises and only resolve them when the headers are resolved', async () => { | |
| const factory = vitest.fn(async () => ({ 'Content-Type': 'application/json' }) as const) | |
| const middlewareMock = vitest.fn(async (builder: HeaderBuilder) => | |
| builder.add('X-Api-Key', '1234'), | |
| ) | |
| const middleware = createHeaderBuilderMiddleware(middlewareMock) | |
| const builder = HeaderBuilder.create().from(factory).with(middleware) | |
| expect(factory).not.toHaveBeenCalled() | |
| expect(middlewareMock).not.toHaveBeenCalled() | |
| const actual = await builder.resolve() | |
| typeCheck<{ 'Content-Type': 'application/json'; 'X-Api-Key': '1234' }>(actual) | |
| expect(factory).toHaveBeenCalled() | |
| expect(actual).toEqual({ 'Content-Type': 'application/json', 'X-Api-Key': '1234' }) | |
| }) | |
| it('should merge two header builders', async () => { | |
| const builder1 = HeaderBuilder.create().add('Content-Type', 'application/json') | |
| const builder2 = HeaderBuilder.create().add('authorization', 'Bearer token') | |
| const mergedBuilder = builder1.merge(builder2) | |
| const actual = await mergedBuilder.resolve() | |
| typeCheck<{ 'Content-Type': 'application/json'; authorization: 'Bearer token' }>(actual) | |
| expect(actual).toEqual({ 'Content-Type': 'application/json', authorization: 'Bearer token' }) | |
| }) | |
| }) | |
| import { createHeaderBuilderMiddleware } from './createHeaderBuilderMiddleware.js' | |
| import { HeaderBuilder } from './headerBuilder.js' | |
| import { describe, expect, vitest } from 'vitest' | |
| // biome-ignore lint/complexity/noBannedTypes: To test empty headers, we need to use an empty object type. | |
| type ExpectedEmptyHeaders = {} | |
| // It is critical that type safety is maintained when refactoring - this offers a way to unit test the type-system. | |
| // The call will fail to build if the expected type is not correct. | |
| const typeCheck = <const Expected>(_actual: Expected): void => {} | |
| const mockGetToken = async () => '1234' | |
| describe('HeaderBuilder', () => { | |
| it('should create an empty header builder', async () => { | |
| const builder = HeaderBuilder.create() | |
| const actual = await builder.resolve() | |
| typeCheck<ExpectedEmptyHeaders>(actual) | |
| expect(actual).toEqual({}) | |
| }) | |
| it('should create a header builder with default headers', async () => { | |
| const builder = HeaderBuilder.create({ 'Content-Type': 'application/json' }) | |
| const actual = await builder.resolve() | |
| typeCheck<{ 'Content-Type': 'application/json' }>(actual) | |
| expect(actual).toEqual({ 'Content-Type': 'application/json' }) | |
| }) | |
| it('should allow individual headers to be added to the builder by name', async () => { | |
| const builder = HeaderBuilder.create().add('Content-Type', 'application/json') | |
| const actual = await builder.resolve() | |
| typeCheck<{ 'Content-Type': 'application/json' }>(actual) | |
| expect(actual).toEqual({ 'Content-Type': 'application/json' }) | |
| }) | |
| it('should allow multiple headers to be added in one static object', async () => { | |
| const builder = HeaderBuilder.create().and({ | |
| 'Content-Type': 'application/json', | |
| 'X-Api-Key': '1234', | |
| }) | |
| const actual = await builder.resolve() | |
| typeCheck<{ 'Content-Type': 'application/json'; 'X-Api-Key': '1234' }>(actual) | |
| expect(actual).toEqual({ 'Content-Type': 'application/json', 'X-Api-Key': '1234' }) | |
| }) | |
| it('should allow multiple headers to be added in a promise of one static object', async () => { | |
| const builder = HeaderBuilder.create().and( | |
| Promise.resolve({ | |
| 'Content-Type': 'application/json', | |
| 'X-Api-Key': '1234', | |
| }), | |
| ) | |
| const actual = await builder.resolve() | |
| typeCheck<{ 'Content-Type': 'application/json'; 'X-Api-Key': '1234' }>(actual) | |
| expect(actual).toEqual({ 'Content-Type': 'application/json', 'X-Api-Key': '1234' }) | |
| }) | |
| it('should allow headers to be added from a factory function', async () => { | |
| const builder = HeaderBuilder.create().from(async () => { | |
| const token = await mockGetToken() | |
| return { authorization: `Bearer ${token}` } | |
| }) | |
| const actual = await builder.resolve() | |
| typeCheck<{ authorization: string }>(actual) | |
| expect(actual).toEqual({ authorization: 'Bearer 1234' }) | |
| }) | |
| it('should allow you to extend the builder with middleware functions', async () => { | |
| const middleware = createHeaderBuilderMiddleware(async (builder) => { | |
| const token = await mockGetToken() | |
| return builder.and({ authorization: `Bearer ${token}` }) | |
| }) | |
| const builder = HeaderBuilder.create().with(middleware) | |
| const actual = await builder.resolve() | |
| typeCheck<{ authorization: string }>(actual) | |
| expect(actual).toEqual({ authorization: 'Bearer 1234' }) | |
| }) | |
| it('should lazy load promises and only resolve them when the headers are resolved', async () => { | |
| const factory = vitest.fn(async () => ({ 'Content-Type': 'application/json' }) as const) | |
| const middlewareMock = vitest.fn(async (builder: HeaderBuilder) => | |
| builder.add('X-Api-Key', '1234'), | |
| ) | |
| const middleware = createHeaderBuilderMiddleware(middlewareMock) | |
| const builder = HeaderBuilder.create().from(factory).with(middleware) | |
| expect(factory).not.toHaveBeenCalled() | |
| expect(middlewareMock).not.toHaveBeenCalled() | |
| const actual = await builder.resolve() | |
| typeCheck<{ 'Content-Type': 'application/json'; 'X-Api-Key': '1234' }>(actual) | |
| expect(factory).toHaveBeenCalled() | |
| expect(actual).toEqual({ 'Content-Type': 'application/json', 'X-Api-Key': '1234' }) | |
| }) | |
| it('should merge two header builders', async () => { | |
| const builder1 = HeaderBuilder.create().add('Content-Type', 'application/json') | |
| const builder2 = HeaderBuilder.create().add('authorization', 'Bearer token') | |
| const mergedBuilder = builder1.merge(builder2) | |
| const actual = await mergedBuilder.resolve() | |
| typeCheck<{ 'Content-Type': 'application/json'; authorization: 'Bearer token' }>(actual) | |
| expect(actual).toEqual({ 'Content-Type': 'application/json', authorization: 'Bearer token' }) | |
| }) | |
| }) |
🤖 Prompt for AI Agents
In packages/app/api-contracts/src/headers/headerBuilder.test.ts from lines 1 to
134, the test suite is comprehensive and well-structured. To improve
organization, consider grouping related tests into nested describe blocks for
better readability and maintainability. For example, group tests for adding
headers, middleware, and merging separately. This will not change functionality
but will enhance clarity and navigation within the test file.
Comment on lines
+5
to
+66
| describe('createHeaderBuilderMiddleware', () => { | ||
| it('should create a middleware that adds a header', async () => { | ||
| const middleware = createHeaderBuilderMiddleware((builder) => | ||
| builder.add('X-Test-Header', 'test-value'), | ||
| ) | ||
|
|
||
| const builder = HeaderBuilder.create().with(middleware) | ||
| const actual = await builder.resolve() | ||
|
|
||
| expect(actual).toEqual({ 'X-Test-Header': 'test-value' }) | ||
| }) | ||
|
|
||
| it('should create a middleware that adds multiple headers', async () => { | ||
| const middleware = createHeaderBuilderMiddleware((builder) => | ||
| builder.and({ | ||
| 'X-Test-Header-1': 'value1', | ||
| 'X-Test-Header-2': 'value2', | ||
| }), | ||
| ) | ||
|
|
||
| const builder = HeaderBuilder.create().with(middleware) | ||
| const actual = await builder.resolve() | ||
|
|
||
| expect(actual).toEqual({ | ||
| 'X-Test-Header-1': 'value1', | ||
| 'X-Test-Header-2': 'value2', | ||
| }) | ||
| }) | ||
|
|
||
| it('should create a middleware that adds headers asynchronously', async () => { | ||
| const middleware = createHeaderBuilderMiddleware(async (builder) => { | ||
| const token = await new Promise<string>((resolve) => resolve('async-token')) | ||
| return builder.add('authorization', `Bearer ${token}`) | ||
| }) | ||
|
|
||
| const builder = HeaderBuilder.create().with(middleware) | ||
| const actual = await builder.resolve() | ||
|
|
||
| expect(actual).toEqual({ authorization: 'Bearer async-token' }) | ||
| }) | ||
|
|
||
| it('should create a middleware that merges headers from another builder', async () => { | ||
| const otherBuilder = HeaderBuilder.create().add('X-Other-Header', 'other-value') | ||
| const middleware = createHeaderBuilderMiddleware((builder) => builder.merge(otherBuilder)) | ||
|
|
||
| const builder = HeaderBuilder.create().with(middleware) | ||
| const actual = await builder.resolve() | ||
|
|
||
| expect(actual).toEqual({ 'X-Other-Header': 'other-value' }) | ||
| }) | ||
|
|
||
| it('should handle middleware that returns a promise of a builder', async () => { | ||
| const middleware = createHeaderBuilderMiddleware((builder) => | ||
| Promise.resolve(builder.add('X-Promise-Header', 'promise-value')), | ||
| ) | ||
|
|
||
| const builder = HeaderBuilder.create().with(middleware) | ||
| const actual = await builder.resolve() | ||
|
|
||
| expect(actual).toEqual({ 'X-Promise-Header': 'promise-value' }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding tests for error scenarios and edge cases
The current tests cover the happy path well, but since this is a draft implementation seeking feedback, consider adding tests for:
- Error handling when middleware throws an exception
- Chaining multiple middleware together
- Header value overriding behavior when the same header is set multiple times
- Type inference validation to ensure the middleware preserves type safety
Would you like me to generate additional test cases for these scenarios?
🤖 Prompt for AI Agents
In packages/app/api-contracts/src/headers/createHeaderBuilderMiddleware.test.ts
between lines 5 and 66, the current tests only cover successful middleware
behavior. To improve coverage, add tests that verify error handling when
middleware throws exceptions, chaining multiple middleware functions to ensure
combined behavior, how header values are overridden when the same header is set
multiple times, and validate type inference to confirm the middleware preserves
type safety. Implement these tests by simulating errors, composing multiple
middleware, setting duplicate headers, and using TypeScript assertions or type
checks.
Comment on lines
41
to
43
| const headers = HeaderBuilder.create('headers' in params ? (params.headers as Headers) : {}) | ||
| .and(contractHeaders) | ||
| .resolve() |
There was a problem hiding this comment.
Missing await for async header resolution
The resolve() method on HeaderBuilder returns a Promise but it's not being awaited.
Apply this fix:
- const headers = HeaderBuilder.create('headers' in params ? (params.headers as Headers) : {})
- .and(contractHeaders)
- .resolve()
+ const headers = await HeaderBuilder.create('headers' in params ? (params.headers as Headers) : {})
+ .and(contractHeaders)
+ .resolve()📝 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.
Suggested change
| const headers = HeaderBuilder.create('headers' in params ? (params.headers as Headers) : {}) | |
| .and(contractHeaders) | |
| .resolve() | |
| const headers = await HeaderBuilder.create('headers' in params ? (params.headers as Headers) : {}) | |
| .and(contractHeaders) | |
| .resolve() |
🤖 Prompt for AI Agents
In packages/app/frontend-http-client/src/contract.ts around lines 41 to 43, the
resolve() method on HeaderBuilder returns a Promise but is not awaited. Fix this
by adding the await keyword before the resolve() call to properly handle the
asynchronous operation and ensure headers are fully resolved before use.
Comment on lines
+269
to
+325
| export type InferGetDetails<Route extends AnyGetRoute> = Route extends GetRouteDefinition< | ||
| infer SuccessResponseBodySchema, | ||
| infer PathParamsSchema, | ||
| infer RequestQuerySchema, | ||
| infer RequestHeaderSchema, | ||
| infer IsNonJSONResponseExpected, | ||
| infer IsEmptyResponseExpected | ||
| > | ||
| ? { | ||
| responseBodySchema: SuccessResponseBodySchema | ||
| pathParamsSchema: PathParamsSchema | ||
| requestQuerySchema: RequestQuerySchema | ||
| requestHeaderSchema: RequestHeaderSchema | ||
| isNonJSONResponseExpected: IsNonJSONResponseExpected | ||
| isEmptyResponseExpected: IsEmptyResponseExpected | ||
| } | ||
| : never | ||
|
|
||
| export type InferDeleteDetails<Route extends AnyDeleteRoute> = Route extends DeleteRouteDefinition< | ||
| infer SuccessResponseBodySchema, | ||
| infer PathParamsSchema, | ||
| infer RequestQuerySchema, | ||
| infer RequestHeaderSchema, | ||
| infer IsNonJSONResponseExpected, | ||
| infer IsEmptyResponseExpected | ||
| > | ||
| ? { | ||
| responseBodySchema: SuccessResponseBodySchema | ||
| pathParamsSchema: PathParamsSchema | ||
| requestQuerySchema: RequestQuerySchema | ||
| requestHeaderSchema: RequestHeaderSchema | ||
| isNonJSONResponseExpected: IsNonJSONResponseExpected | ||
| isEmptyResponseExpected: IsEmptyResponseExpected | ||
| } | ||
| : never | ||
|
|
||
| export type InferPayloadDetails<Route extends AnyPayloadRoute> = | ||
| Route extends PayloadRouteDefinition< | ||
| infer RequestBodySchema, | ||
| infer SuccessResponseBodySchema, | ||
| infer PathParamsSchema, | ||
| infer RequestQuerySchema, | ||
| infer RequestHeaderSchema, | ||
| infer IsNonJSONResponseExpected, | ||
| infer IsEmptyResponseExpected | ||
| > | ||
| ? { | ||
| requestBodySchema: RequestBodySchema | ||
| responseBodySchema: SuccessResponseBodySchema | ||
| pathParamsSchema: PathParamsSchema | ||
| requestQuerySchema: RequestQuerySchema | ||
| requestHeaderSchema: RequestHeaderSchema | ||
| isNonJSONResponseExpected: IsNonJSONResponseExpected | ||
| isEmptyResponseExpected: IsEmptyResponseExpected | ||
| } | ||
| : never | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add JSDoc documentation for the new inference types
These type utilities would benefit from documentation explaining their purpose and usage.
Example documentation:
+/**
+ * Extracts type information from a GET route definition including schemas,
+ * parameters, and response metadata
+ * @example
+ * type Details = InferGetDetails<typeof myGetRoute>
+ */
export type InferGetDetails<Route extends AnyGetRoute> = Route extends GetRouteDefinition<📝 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.
Suggested change
| export type InferGetDetails<Route extends AnyGetRoute> = Route extends GetRouteDefinition< | |
| infer SuccessResponseBodySchema, | |
| infer PathParamsSchema, | |
| infer RequestQuerySchema, | |
| infer RequestHeaderSchema, | |
| infer IsNonJSONResponseExpected, | |
| infer IsEmptyResponseExpected | |
| > | |
| ? { | |
| responseBodySchema: SuccessResponseBodySchema | |
| pathParamsSchema: PathParamsSchema | |
| requestQuerySchema: RequestQuerySchema | |
| requestHeaderSchema: RequestHeaderSchema | |
| isNonJSONResponseExpected: IsNonJSONResponseExpected | |
| isEmptyResponseExpected: IsEmptyResponseExpected | |
| } | |
| : never | |
| export type InferDeleteDetails<Route extends AnyDeleteRoute> = Route extends DeleteRouteDefinition< | |
| infer SuccessResponseBodySchema, | |
| infer PathParamsSchema, | |
| infer RequestQuerySchema, | |
| infer RequestHeaderSchema, | |
| infer IsNonJSONResponseExpected, | |
| infer IsEmptyResponseExpected | |
| > | |
| ? { | |
| responseBodySchema: SuccessResponseBodySchema | |
| pathParamsSchema: PathParamsSchema | |
| requestQuerySchema: RequestQuerySchema | |
| requestHeaderSchema: RequestHeaderSchema | |
| isNonJSONResponseExpected: IsNonJSONResponseExpected | |
| isEmptyResponseExpected: IsEmptyResponseExpected | |
| } | |
| : never | |
| export type InferPayloadDetails<Route extends AnyPayloadRoute> = | |
| Route extends PayloadRouteDefinition< | |
| infer RequestBodySchema, | |
| infer SuccessResponseBodySchema, | |
| infer PathParamsSchema, | |
| infer RequestQuerySchema, | |
| infer RequestHeaderSchema, | |
| infer IsNonJSONResponseExpected, | |
| infer IsEmptyResponseExpected | |
| > | |
| ? { | |
| requestBodySchema: RequestBodySchema | |
| responseBodySchema: SuccessResponseBodySchema | |
| pathParamsSchema: PathParamsSchema | |
| requestQuerySchema: RequestQuerySchema | |
| requestHeaderSchema: RequestHeaderSchema | |
| isNonJSONResponseExpected: IsNonJSONResponseExpected | |
| isEmptyResponseExpected: IsEmptyResponseExpected | |
| } | |
| : never | |
| /** | |
| * Extracts type information from a GET route definition including schemas, | |
| * parameters, and response metadata | |
| * @example | |
| * type Details = InferGetDetails<typeof myGetRoute> | |
| */ | |
| export type InferGetDetails<Route extends AnyGetRoute> = Route extends GetRouteDefinition< | |
| infer SuccessResponseBodySchema, | |
| infer PathParamsSchema, | |
| infer RequestQuerySchema, | |
| infer RequestHeaderSchema, | |
| infer IsNonJSONResponseExpected, | |
| infer IsEmptyResponseExpected | |
| > | |
| ? { | |
| responseBodySchema: SuccessResponseBodySchema | |
| pathParamsSchema: PathParamsSchema | |
| requestQuerySchema: RequestQuerySchema | |
| requestHeaderSchema: RequestHeaderSchema | |
| isNonJSONResponseExpected: IsNonJSONResponseExpected | |
| isEmptyResponseExpected: IsEmptyResponseExpected | |
| } | |
| : never |
🤖 Prompt for AI Agents
In packages/app/api-contracts/src/apiContracts.ts between lines 269 and 325, the
new type inference utilities lack JSDoc comments explaining their purpose and
usage. Add clear JSDoc documentation above each type definition describing what
the type infers, the generic parameters it accepts, and what the resulting type
represents to improve code readability and maintainability.
| const headers = this.factories.map((header) => header()) | ||
| const resolvedHeaders = await Promise.all(headers) | ||
|
|
||
| return Object.assign({}, ...resolvedHeaders) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document header merging behavior.
The Object.assign({}, ...resolvedHeaders) will silently override headers with the same key. Later headers in the array will override earlier ones. This behavior should be documented in the method's JSDoc comment.
Add documentation about the merging behavior:
/**
* Resolves the headers by waiting for all the promises to resolve and merging them together.
+ * When multiple headers have the same key, later headers will override earlier ones.
*
* @example📝 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.
Suggested change
| return Object.assign({}, ...resolvedHeaders) | |
| /** | |
| * Resolves the headers by waiting for all the promises to resolve and merging them together. | |
| * When multiple headers have the same key, later headers will override earlier ones. | |
| * | |
| * @example | |
| * // … | |
| */ |
🤖 Prompt for AI Agents
In packages/app/api-contracts/src/headers/headerBuilder.ts at line 209, the
method uses Object.assign({}, ...resolvedHeaders) to merge headers, which
silently overrides headers with the same key, with later headers taking
precedence. Update the JSDoc comment of this method to explicitly document this
merging behavior, stating that headers with duplicate keys are overridden by
later entries in the array.
Comment on lines
+57
to
+59
| static create<const H extends Headers>(initialHeaders = {} as H): HeaderBuilder<H> { | ||
| return new HeaderBuilder([() => initialHeaders]) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider validating initial headers parameter.
The static create method accepts initialHeaders which could be a function, promise, or object. However, if a function is passed, it's wrapped in another function, which could lead to unexpected behavior.
Consider handling different input types explicitly:
-static create<const H extends Headers>(initialHeaders = {} as H): HeaderBuilder<H> {
- return new HeaderBuilder([() => initialHeaders])
+static create<const H extends Headers>(initialHeaders = {} as H): HeaderBuilder<H> {
+ const factory = typeof initialHeaders === 'function' ? initialHeaders : () => initialHeaders
+ return new HeaderBuilder([factory])
}📝 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.
Suggested change
| static create<const H extends Headers>(initialHeaders = {} as H): HeaderBuilder<H> { | |
| return new HeaderBuilder([() => initialHeaders]) | |
| } | |
| static create<const H extends Headers>(initialHeaders = {} as H): HeaderBuilder<H> { | |
| const factory = typeof initialHeaders === 'function' | |
| ? initialHeaders | |
| : () => initialHeaders | |
| return new HeaderBuilder([factory]) | |
| } |
🤖 Prompt for AI Agents
In packages/app/api-contracts/src/headers/headerBuilder.ts around lines 57 to
59, the static create method currently wraps the initialHeaders parameter in a
function without checking its type, which can cause issues if initialHeaders is
already a function or a promise. To fix this, add explicit type checks for
initialHeaders: if it is a function, use it directly; if it is a promise, handle
it appropriately; otherwise, wrap it in a function returning the object. This
ensures consistent and expected behavior regardless of the input type.
Comment on lines
17
to
18
| import type { Wretch, WretchResponse } from 'wretch' | ||
| import type { Wretch } from 'wretch/dist/cjs/index.js' |
There was a problem hiding this comment.
Remove duplicate import statement.
There's a duplicate import of Wretch type with different import paths. This will cause a compilation error.
Remove the duplicate import:
import type { Wretch, WretchResponse } from 'wretch'
-import type { Wretch } from 'wretch/dist/cjs/index.js'📝 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.
Suggested change
| import type { Wretch, WretchResponse } from 'wretch' | |
| import type { Wretch } from 'wretch/dist/cjs/index.js' | |
| import type { Wretch, WretchResponse } from 'wretch' |
🤖 Prompt for AI Agents
In packages/app/frontend-http-client/src/types.ts around lines 17 to 18, there
are duplicate import statements for the Wretch type from different paths. Remove
one of the imports to avoid compilation errors, keeping only a single import
statement for Wretch and WretchResponse from the correct and consistent module
path.
Comment on lines
61
to
65
| definition: Definition, | ||
| clientResolver: (service: string) => Promise<Client>, | ||
| contractHeaders?: HeaderBuilder<ContractHeaders>, | ||
| ): ConfiguredContractService<Client, Definition, ContractHeaders> { | ||
| const clientCache = clientResolver(definition.serviceName) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding error handling for client resolver.
The clientResolver promise is not wrapped in try-catch. If it rejects, all routes for this contract will fail.
Consider adding error handling:
export function configureContractService<
Client,
Definition extends ContractDefinitions<AnyRoutes>,
ContractHeaders extends Headers = NoHeaders,
>(
definition: Definition,
clientResolver: (service: string) => Promise<Client>,
contractHeaders?: HeaderBuilder<ContractHeaders>,
): ConfiguredContractService<Client, Definition, ContractHeaders> {
- const clientCache = clientResolver(definition.serviceName)
+ const clientCache = clientResolver(definition.serviceName).catch(error => {
+ throw new Error(`Failed to resolve client for service ${definition.serviceName}: ${error.message}`)
+ })
const contractHeadersBuilder = contractHeaders ?? HeaderBuilder.create<ContractHeaders>()📝 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.
Suggested change
| definition: Definition, | |
| clientResolver: (service: string) => Promise<Client>, | |
| contractHeaders?: HeaderBuilder<ContractHeaders>, | |
| ): ConfiguredContractService<Client, Definition, ContractHeaders> { | |
| const clientCache = clientResolver(definition.serviceName) | |
| export function configureContractService< | |
| Client, | |
| Definition extends ContractDefinitions<AnyRoutes>, | |
| ContractHeaders extends Headers = NoHeaders, | |
| >( | |
| definition: Definition, | |
| clientResolver: (service: string) => Promise<Client>, | |
| contractHeaders?: HeaderBuilder<ContractHeaders>, | |
| ): ConfiguredContractService<Client, Definition, ContractHeaders> { | |
| const clientCache = clientResolver(definition.serviceName).catch(error => { | |
| throw new Error(`Failed to resolve client for service ${definition.serviceName}: ${error.message}`) | |
| }) | |
| const contractHeadersBuilder = contractHeaders ?? HeaderBuilder.create<ContractHeaders>() |
🤖 Prompt for AI Agents
In packages/app/api-contracts/src/contractService.ts around lines 61 to 65, the
clientResolver promise is called without error handling, which can cause all
routes to fail if the promise rejects. Wrap the clientResolver call in a
try-catch block to handle potential rejections gracefully, and implement
appropriate error handling or fallback logic inside the catch block to prevent
the entire contract service from failing.
Contributor
Author
There was a problem hiding this comment.
NOTE: This header stuff was copied from harmony.. It might be over kill since we can provide global hearers at the config level - but I heave left it in for now, for simplicity.
Comment on lines
1
to
13
| import { ContractDefinition, AvailableService, type AnyRouter, type Headers } from '@lokalise/api-contracts' | ||
|
|
||
| function initialiseContractService() { | ||
|
|
||
| } | ||
|
|
||
| function createServiceMethod< | ||
| Service extends AvailableService, | ||
| Router extends AnyRouter, | ||
| ContractHeaders extends Headers, | ||
| >(definition: ContractDefinition<Service, Router, ContractHeaders>) { | ||
|
|
||
| } No newline at end of file |
Contributor
Author
There was a problem hiding this comment.
Oops! Shouldn't have pushed this 😅
| InferSchemaOutput, | ||
| } from '@lokalise/api-contracts' | ||
| import type { Wretch, WretchResponse } from 'wretch' | ||
| import type { Wretch } from 'wretch/dist/cjs/index.js' |
Contributor
Author
There was a problem hiding this comment.
:harold-pain:
| ContractDefinitions<AnyRoutes>, | ||
| ContractHeaders | ||
| >, | ||
| >(contract: Configured): ContractService<Configured> { |
Contributor
Author
There was a problem hiding this comment.
Thinking about it - this should also take in an optional global headers type parameter so that we can set headers for all routes at the top level, but in the client side - at the point of configuration
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
This is a first draft of the changes described in the Contract Service Abstraction proposal.
I would like to start a discussion about what's missing and what might be needed, and this is a good reference aide for those discussions. Please do feel free to leave comments, suggestions and feedback though.
Checklist
major,minor,patchorskip-release