fix(fetch): use current global fetch instead of cached one when env f…#2
fix(fetch): use current global fetch instead of cached one when env f…#2MitchLewis930 wants to merge 1 commit intopr_022_beforefrom
Conversation
…etch is not specified to keep MSW support; (axios#7030)
📝 WalkthroughWalkthroughThe fetch adapter is refactored to implement environment-based fetch resolution with fallback support. Environment configuration now prefers an explicit envFetch while defaulting to an empty object when no env is provided. The adapter uses a merged environment to select the appropriate fetch implementation, supporting cases where fetch is unavailable or overridden. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/adapters/fetch.js`:
- Around line 265-266: The destructuring of {fetch, Request, Response} assumes
env is an object but env can be undefined when config.env is missing; update the
assignment that creates env (used by the fetch adapter) to default to an empty
object (e.g., env = (config && config.env) || {}) so the subsequent destructure
in lib/adapters/fetch.js cannot throw; change the code referencing env (the
variables fetch, Request, Response) used by getFetch to always read from a
guaranteed object.
In `@test/unit/adapters/fetch.js`:
- Around line 493-517: The test currently reassigns the global fetch variable
(using variable globalFetch and direct fetch = ...) which triggers
noGlobalAssign; change to capture and restore globalThis.fetch instead: store
the original as const originalFetch = globalThis.fetch, replace with
globalThis.fetch = async () => {...} before calling startHTTPServer and
fetchAxios.get, and in the finally block restore globalThis.fetch =
originalFetch; keep the rest of the test logic (env: { fetch: undefined },
assertions, and use of startHTTPServer and fetchAxios.get) unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/adapters/fetch.jstest/unit/adapters/fetch.js
🧰 Additional context used
🧬 Code graph analysis (2)
lib/adapters/fetch.js (1)
lib/utils.js (1)
isFunction(95-95)
test/unit/adapters/fetch.js (1)
test/helpers/server.js (2)
startHTTPServer(14-53)startHTTPServer(14-53)
🪛 Biome (2.3.13)
test/unit/adapters/fetch.js
[error] 496-496: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
[error] 516-516: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
🔇 Additional comments (2)
lib/adapters/fetch.js (2)
15-38: LGTM.
The adapter setup and env merge flow look coherent.
141-142: LGTM — consistent resolved transport usage.Also applies to: 202-202
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| let env = config ? config.env : {}; | ||
| const {fetch, Request, Response} = env; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files lib/adapters/fetch.jsRepository: Signal65/axios-CodeRabbit
Length of output: 90
🏁 Script executed:
head -n 300 lib/adapters/fetch.js | tail -n 50Repository: Signal65/axios-CodeRabbit
Length of output: 1138
🏁 Script executed:
rg -n -C2 '\bgetFetch\s*\('Repository: Signal65/axios-CodeRabbit
Length of output: 255
🏁 Script executed:
rg -n 'getFetch' --type jsRepository: Signal65/axios-CodeRabbit
Length of output: 246
🏁 Script executed:
rg -n 'getFetch\s*\(' --type js -A 5Repository: Signal65/axios-CodeRabbit
Length of output: 199
🏁 Script executed:
cat lib/adapters/adapters.jsRepository: Signal65/axios-CodeRabbit
Length of output: 2142
Guard against config.env being undefined.
When getFetch is called with a config object (line 60-61 in lib/adapters/adapters.js), the current code at line 265 can set env to undefined if config.env is missing or falsy. This causes line 266 destructuring to throw a TypeError. The suggested fix ensures env always defaults to an object:
✅ Safer defaulting
- let env = config ? config.env : {};
+ let env = (config && config.env) || {};🤖 Prompt for AI Agents
In `@lib/adapters/fetch.js` around lines 265 - 266, The destructuring of {fetch,
Request, Response} assumes env is an object but env can be undefined when
config.env is missing; update the assignment that creates env (used by the fetch
adapter) to default to an empty object (e.g., env = (config && config.env) ||
{}) so the subsequent destructure in lib/adapters/fetch.js cannot throw; change
the code referencing env (the variables fetch, Request, Response) used by
getFetch to always read from a guaranteed object.
| it('should use current global fetch when env fetch is not specified', async() => { | ||
| const globalFetch = fetch; | ||
|
|
||
| fetch = async () => { | ||
| return { | ||
| headers: { | ||
| foo: '1' | ||
| }, | ||
| text: async () => 'global' | ||
| } | ||
| }; | ||
|
|
||
| try { | ||
| server = await startHTTPServer((req, res) => res.end('OK')); | ||
|
|
||
| const {data} = await fetchAxios.get('/', { | ||
| env: { | ||
| fetch: undefined | ||
| } | ||
| }); | ||
|
|
||
| assert.strictEqual(data, 'global'); | ||
| } finally { | ||
| fetch = globalFetch; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the exact file and lines in question
wc -l test/unit/adapters/fetch.jsRepository: Signal65/axios-CodeRabbit
Length of output: 100
🏁 Script executed:
# Read the specific lines mentioned in the review (493-517)
sed -n '493,517p' test/unit/adapters/fetch.jsRepository: Signal65/axios-CodeRabbit
Length of output: 649
🏁 Script executed:
# Search for other global fetch stubs that might need the same treatment
rg -n -C2 '\bfetch\s*=' test/unit --max-count 20Repository: Signal65/axios-CodeRabbit
Length of output: 598
🏁 Script executed:
# Check if there's a eslint/biome configuration file to confirm the noGlobalAssign rule
fd -e .biomerc -e biome.json -e .eslintrc -e .eslintignore 2>/dev/null | head -5Repository: Signal65/axios-CodeRabbit
Length of output: 51
🏁 Script executed:
# Check for biome or eslint config in the root directory
ls -la | grep -E '(biome|eslint)'Repository: Signal65/axios-CodeRabbit
Length of output: 199
🏁 Script executed:
# Check if biome is configured in package.json
cat package.json | grep -A10 -B2 'biome'Repository: Signal65/axios-CodeRabbit
Length of output: 51
🏁 Script executed:
# Search the entire codebase for other uses of globalThis.fetch
rg 'globalThis\.fetch' --max-count 10Repository: Signal65/axios-CodeRabbit
Length of output: 51
🏁 Script executed:
# Check if there are any other test files using globalThis for stubbing
rg 'globalThis\s*\.' test/ --max-count 15Repository: Signal65/axios-CodeRabbit
Length of output: 51
🏁 Script executed:
# Read the biome.json file to check noGlobalAssign configuration
cat biome.jsonRepository: Signal65/axios-CodeRabbit
Length of output: 1844
🏁 Script executed:
# Also check the .eslintrc.cjs file
cat .eslintrc.cjsRepository: Signal65/axios-CodeRabbit
Length of output: 317
🏁 Script executed:
# Final verification: check if there are any other fetch reassignments in the entire test directory
rg 'fetch\s*=' test/ --type js -B2 -A2Repository: Signal65/axios-CodeRabbit
Length of output: 558
Use globalThis.fetch instead of direct global assignment to avoid noGlobalAssign lint violation.
Biome and ESLint both flag direct fetch reassignment as noGlobalAssign. Using globalThis.fetch at lines 496 and 516 resolves the lint error and keeps the global override explicit and testable.
🛠️ Suggested fix
- const globalFetch = fetch;
-
- fetch = async () => {
+ const globalFetch = globalThis.fetch;
+
+ globalThis.fetch = async () => {
return {
headers: {
foo: '1'
},
text: async () => 'global'
}
};
@@
- fetch = globalFetch;
+ globalThis.fetch = globalFetch;
}📝 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.
| it('should use current global fetch when env fetch is not specified', async() => { | |
| const globalFetch = fetch; | |
| fetch = async () => { | |
| return { | |
| headers: { | |
| foo: '1' | |
| }, | |
| text: async () => 'global' | |
| } | |
| }; | |
| try { | |
| server = await startHTTPServer((req, res) => res.end('OK')); | |
| const {data} = await fetchAxios.get('/', { | |
| env: { | |
| fetch: undefined | |
| } | |
| }); | |
| assert.strictEqual(data, 'global'); | |
| } finally { | |
| fetch = globalFetch; | |
| } | |
| it('should use current global fetch when env fetch is not specified', async() => { | |
| const globalFetch = globalThis.fetch; | |
| globalThis.fetch = async () => { | |
| return { | |
| headers: { | |
| foo: '1' | |
| }, | |
| text: async () => 'global' | |
| } | |
| }; | |
| try { | |
| server = await startHTTPServer((req, res) => res.end('OK')); | |
| const {data} = await fetchAxios.get('/', { | |
| env: { | |
| fetch: undefined | |
| } | |
| }); | |
| assert.strictEqual(data, 'global'); | |
| } finally { | |
| globalThis.fetch = globalFetch; | |
| } |
🧰 Tools
🪛 Biome (2.3.13)
[error] 496-496: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
[error] 516-516: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
🤖 Prompt for AI Agents
In `@test/unit/adapters/fetch.js` around lines 493 - 517, The test currently
reassigns the global fetch variable (using variable globalFetch and direct fetch
= ...) which triggers noGlobalAssign; change to capture and restore
globalThis.fetch instead: store the original as const originalFetch =
globalThis.fetch, replace with globalThis.fetch = async () => {...} before
calling startHTTPServer and fetchAxios.get, and in the finally block restore
globalThis.fetch = originalFetch; keep the rest of the test logic (env: { fetch:
undefined }, assertions, and use of startHTTPServer and fetchAxios.get)
unchanged.
2 participants
PR_022
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.