-
Notifications
You must be signed in to change notification settings - Fork 6
feat: add request deduplication to API client #84
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
base: main
Are you sure you want to change the base?
Conversation
- Add promise-sharing deduplication for GET requests in api/client.ts - Implement 2-second deduplication window with automatic cleanup - Normalize URLs with sorted query params for consistent cache keys - Update GlobalState.tsx to use api.getJSON for products endpoint - Update Product.tsx to use api.getJSON for product details endpoint - Add comprehensive E2E tests to verify no duplicate concurrent requests Fixes #80 Amp-Thread-ID: https://ampcode.com/threads/T-3c3a93a0-1e22-4e7c-9093-f155c6b44368 Co-authored-by: Amp <[email protected]>
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.
Summary
This PR refactors the API client to add request deduplication for GET requests and consolidates fetch logic across the application. The changes replace manual fetch calls in GlobalState.tsx and Product.tsx with the centralized api.getJSON method, removing hardcoded API URLs.
Key changes:
- Implements GET request deduplication with a 2-second window using an in-flight request cache
- Adds query parameter normalization to ensure consistent cache keys
- Exposes a public
getJSONmethod for typed API calls - Refactors existing fetch calls to use the new centralized client
Additional considerations:
The deduplication implementation is generally sound with proper cleanup handling via both finally blocks and failsafe timers. However, the type safety concern with Promise<unknown> storage and the potential race condition should be addressed to ensure robustness. The functionality itself should work correctly for typical use cases where the same endpoint consistently returns the same data shape.
|
|
||
| const DEDUPE_INTERVAL_MS = 2000 | ||
|
|
||
| type InFlightEntry = { |
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.
Type safety issue: Storing Promise<unknown> but casting to Promise<T> when reusing can lead to type mismatches. If two different parts of the code request the same endpoint with different expected return types within the deduplication window, the second caller will receive incorrectly typed data from the first request's promise, potentially causing runtime errors.
| } | ||
|
|
||
| // Shared JSON request with GET-deduping | ||
| private async request<T>(endpoint: string, options: globalThis.RequestInit = {}): Promise<T> { |
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.
Unnecessary .toString() call. The options.method is already a string type (from RequestInit), and the fallback 'GET' is also a string. This can be simplified to const method = (options.method || 'GET').toUpperCase()
| private async request<T>(endpoint: string, options: globalThis.RequestInit = {}): Promise<T> { | |
| const method = (options.method || 'GET').toUpperCase() |
| if (method === 'GET') { | ||
| const key = this.buildKey(endpoint, method) | ||
| const now = Date.now() | ||
| const existing = this.inFlight.get(key) |
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.
Potential race condition: If two identical GET requests are initiated simultaneously before either is added to the inFlight map, both will create separate fetch calls, defeating the deduplication mechanism. Consider checking and setting the map entry atomically, or document this as an acceptable limitation for client-side deduplication.
Overview
Implements request deduplication for the API client to eliminate duplicate concurrent requests, addressing issue #80.
Changes
api/client.ts: Added promise-sharing deduplication mechanism for GET requests
GlobalState.tsx: Updated to use
api.getJSONinstead of raw fetchProduct.tsx: Updated to use
api.getJSONinstead of raw fetchE2E Tests: Added comprehensive tests to verify no duplicate concurrent requests
Implementation Details
Follows OSS best practices from TanStack Query, SWR, and Apollo Client:
Testing
Verification
Console logs from E2E tests confirm:
/products?include_delivery_summary=true: 1 request (was 2)/api/categories: 1 request (was 2)/products/{id}: 1 request (was 2)Fixes #80