feat: add opt-in request deduplication (dedupe option)#557
Open
productdevbook wants to merge 2 commits into
Open
feat: add opt-in request deduplication (dedupe option)#557productdevbook wants to merge 2 commits into
productdevbook wants to merge 2 commits into
Conversation
Add `dedupe` option that coalesces identical concurrent GET/HEAD requests into a single network call, sharing the response between all callers. Only applies to non-payload methods. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdded optional request deduplication to Changes
Sequence DiagramsequenceDiagram
participant C1 as Caller 1
participant C2 as Caller 2
participant C3 as Caller 3
participant Fetch as $fetch (with dedupe)
participant Pending as _pendingRequests Map
participant Net as Network
C1->>Fetch: GET /api/data (dedupe: true)
Fetch->>Pending: check "GET:/api/data"
Pending-->>Fetch: not found
Fetch->>Net: issue network request (store promise in Pending)
C2->>Fetch: GET /api/data (dedupe: true)
Fetch->>Pending: check "GET:/api/data"
Pending-->>Fetch: found (pending promise)
Fetch-->>C2: await existing promise
C3->>Fetch: GET /api/data (dedupe: true)
Fetch->>Pending: check "GET:/api/data"
Pending-->>Fetch: found (pending promise)
Fetch-->>C3: await existing promise
Net-->>Fetch: response/error
Fetch-->>C1: return result/error
Fetch-->>C2: return result/error
Fetch-->>C3: return result/error
Fetch->>Pending: delete "GET:/api/data" (finally)
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
🧹 Nitpick comments (2)
src/fetch.ts (1)
266-269: Coalesced requests silently ignore differing options.When a request coalesces with an in-flight request, the second caller's options (
parseResponse,responseType, hooks, headers) are completely ignored. The first caller's options determine how the response is processed.This could lead to subtle bugs if callers expect their own options to be respected:
// Caller A's parseResponse is used $fetch('/api', { dedupe: true, parseResponse: parserA }) // Caller B's parseResponse is silently ignored $fetch('/api', { dedupe: true, parseResponse: parserB })Consider either:
- Documenting this behavior prominently in the JSDoc
- Only coalescing when options that affect response processing are equivalent
- Logging a warning in development when options differ
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fetch.ts` around lines 266 - 269, The current coalescing logic returns the in-flight promise (pending) and uses its processed result (r._data) without considering the second caller's options, so callers' parseResponse/responseType/hooks/headers can be ignored; modify the coalescing to only await and return pending when the new call's effective response-processing options exactly match the in-flight call's options (compare parseResponse, responseType, hooks and relevant headers using a shallow equality or normalized canonical form), otherwise do not reuse pending and start a new request; in the code around pending and the r._data return, add an options-equality check (e.g., isSameResponseOptions(inflightOptions, currentOptions)) before returning r._data, and fall through to issue a separate fetch when they differ.test/index.test.ts (1)
528-576: Good test coverage, but consider adding edge case tests.The test suite covers the happy path well. Consider adding tests for:
Query params affecting dedupe key (related to the bug above):
it("does not dedupe requests with different query params", async () => { await Promise.all([ $fetch(getURL("params"), { query: { a: 1 }, dedupe: true }), $fetch(getURL("params"), { query: { b: 2 }, dedupe: true }), ]); expect(fetch).toHaveBeenCalledTimes(2); });baseURL resolution:
it("dedupes requests that resolve to the same URL via baseURL", async () => { const base = getURL(""); await Promise.all([ $fetch("/ok", { baseURL: base, dedupe: true }), $fetch("/ok", { baseURL: base, dedupe: true }), ]); expect(fetch).toHaveBeenCalledOnce(); });Error propagation to ensure all coalesced callers receive the error:
it("propagates errors to all coalesced callers", async () => { const url = getURL("403"); const results = await Promise.allSettled([ $fetch(url, { dedupe: true }), $fetch(url, { dedupe: true }), ]); expect(results.every(r => r.status === "rejected")).toBe(true); expect(fetch).toHaveBeenCalledOnce(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index.test.ts` around lines 528 - 576, Add edge-case tests for the request deduplication suite to cover query-param sensitivity, baseURL resolution, and error propagation: add tests using $fetch and getURL that verify different query params do not coalesce (expect fetch called twice), that requests resolved via baseURL (e.g. $fetch("/ok", { baseURL: base, dedupe: true })) do coalesce into a single fetch call, and that when the underlying fetch for getURL("403") errors all coalesced callers see the rejection (use Promise.allSettled and assert every result is rejected); reference the existing "request deduplication" describe block and reuse $fetch, getURL and the mocked fetch assertions (fetch.mockClear(), toHaveBeenCalledOnce/Times) to implement these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fetch.ts`:
- Around line 261-278: The dedupe key is computed from the raw request before
$fetchRaw applies baseURL/query; change the logic so the dedupe key is computed
from the resolved/final request URL (including baseURL and serialized query) and
use that key for both the _pendingRequests lookup and set; in practice, call
$fetchRaw (or the internal request resolution routine) to obtain the resolved
request/response object and derive the key from its final URL (referencing
options?.dedupe, isPayloadMethod, $fetchRaw, and _pendingRequests), then check
_pendingRequests.get(resolvedKey) and set/delete _pendingRequests using
resolvedKey so identical final network requests are correctly coalesced.
---
Nitpick comments:
In `@src/fetch.ts`:
- Around line 266-269: The current coalescing logic returns the in-flight
promise (pending) and uses its processed result (r._data) without considering
the second caller's options, so callers'
parseResponse/responseType/hooks/headers can be ignored; modify the coalescing
to only await and return pending when the new call's effective
response-processing options exactly match the in-flight call's options (compare
parseResponse, responseType, hooks and relevant headers using a shallow equality
or normalized canonical form), otherwise do not reuse pending and start a new
request; in the code around pending and the r._data return, add an
options-equality check (e.g., isSameResponseOptions(inflightOptions,
currentOptions)) before returning r._data, and fall through to issue a separate
fetch when they differ.
In `@test/index.test.ts`:
- Around line 528-576: Add edge-case tests for the request deduplication suite
to cover query-param sensitivity, baseURL resolution, and error propagation: add
tests using $fetch and getURL that verify different query params do not coalesce
(expect fetch called twice), that requests resolved via baseURL (e.g.
$fetch("/ok", { baseURL: base, dedupe: true })) do coalesce into a single fetch
call, and that when the underlying fetch for getURL("403") errors all coalesced
callers see the rejection (use Promise.allSettled and assert every result is
rejected); reference the existing "request deduplication" describe block and
reuse $fetch, getURL and the mocked fetch assertions (fetch.mockClear(),
toHaveBeenCalledOnce/Times) to implement these tests.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6729e79a-3fcb-4dbb-acb3-c28dcc74e231
📒 Files selected for processing (3)
src/fetch.tssrc/types.tstest/index.test.ts
Comment on lines
+261
to
+278
| // Request deduplication for concurrent identical requests | ||
| if (options?.dedupe && !isPayloadMethod(options?.method)) { | ||
| const method = (options?.method || "GET").toUpperCase(); | ||
| const key = `${method}:${typeof request === "string" ? request : (request as Request).url}`; | ||
| const pending = _pendingRequests.get(key); | ||
| if (pending) { | ||
| const r = await pending; | ||
| return r._data; | ||
| } | ||
| const promise = $fetchRaw(request, options); | ||
| _pendingRequests.set(key, promise); | ||
| try { | ||
| const r = await promise; | ||
| return r._data; | ||
| } finally { | ||
| _pendingRequests.delete(key); | ||
| } | ||
| } |
There was a problem hiding this comment.
Dedupe key is computed before URL resolution, causing incorrect coalescing.
The key is computed from the raw request URL before $fetchRaw applies baseURL and query transformations. This causes two problems:
-
Different queries, same key: Requests with different
queryparams get the same key and incorrectly share responses:// Both get key "GET:/api" but should NOT dedupe $fetch('/api', { query: { a: 1 }, dedupe: true }) $fetch('/api', { query: { b: 2 }, dedupe: true })
-
Same final URL, different keys: Requests resolving to the same URL may not dedupe:
// Different keys but resolve to same URL $fetch('/api', { baseURL: 'https://x.com', dedupe: true }) // key: GET:/api $fetch('https://x.com/api', { dedupe: true }) // key: GET:https://x.com/api
The key should be computed after URL resolution to match the actual network request identity.
🐛 Proposed fix: compute key after URL resolution
const $fetch = async function $fetch(request, options) {
// Request deduplication for concurrent identical requests
if (options?.dedupe && !isPayloadMethod(options?.method)) {
const method = (options?.method || "GET").toUpperCase();
- const key = `${method}:${typeof request === "string" ? request : (request as Request).url}`;
+ // Resolve URL with baseURL and query before computing dedupe key
+ let resolvedUrl = typeof request === "string" ? request : (request as Request).url;
+ if (typeof resolvedUrl === "string") {
+ if (options?.baseURL) {
+ resolvedUrl = withBase(resolvedUrl, options.baseURL);
+ }
+ if (options?.query) {
+ resolvedUrl = withQuery(resolvedUrl, options.query);
+ }
+ }
+ const key = `${method}:${resolvedUrl}`;
const pending = _pendingRequests.get(key);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fetch.ts` around lines 261 - 278, The dedupe key is computed from the raw
request before $fetchRaw applies baseURL/query; change the logic so the dedupe
key is computed from the resolved/final request URL (including baseURL and
serialized query) and use that key for both the _pendingRequests lookup and set;
in practice, call $fetchRaw (or the internal request resolution routine) to
obtain the resolved request/response object and derive the key from its final
URL (referencing options?.dedupe, isPayloadMethod, $fetchRaw, and
_pendingRequests), then check _pendingRequests.get(resolvedKey) and set/delete
_pendingRequests using resolvedKey so identical final network requests are
correctly coalesced.
…test - Move deduplication logic into $fetchRaw so it works for both $fetch() and $fetch.raw() - Compute dedupe key AFTER URL resolution (baseURL + query applied), so different query params get different keys - Extract _executeFetch for the non-dedupe path - Add tests: different query params not coalesced, error propagation, $fetch.raw dedupe support Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
dedupeoption to coalesce identical concurrent requests into a single network callUsage
How it works
METHOD:URL(after baseURL/query resolution happens inside $fetchRaw)createFetchinstancefinallyblock — subsequent requests make new network callsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
dedupeoption, concurrent identical non-payload requests (e.g., GET/HEAD) to the same URL are coalesced so callers share a single network response. Deduplication is scoped by full URL (including query string) and does not apply to payload methods (e.g., POST).Tests