Skip to content

fix(fetch): use current global fetch instead of cached one when env f… #2

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.

Sign up for GitHub

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

Open
MitchLewis930 wants to merge 1 commit into
base: pr_022_before
Choose a base branch
from
+40 −11
Open

fix(fetch): use current global fetch instead of cached one when env f… #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 13 additions & 11 deletions lib/adapters/fetch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ const DEFAULT_CHUNK_SIZE = 64 * 1024;

const {isFunction} = utils;

const globalFetchAPI = (({fetch, Request, Response}) => ({
fetch, Request, Response
}))(utils.global);
const globalFetchAPI = (({Request, Response}) => ({
Request, Response
}))(utils.global);

const {
ReadableStream, TextEncoder
Expand All @@ -30,8 +30,12 @@ const test = (fn, ...args) => {
}

const factory = (env) => {
const {fetch, Request, Response} = Object.assign({}, globalFetchAPI, env);
const isFetchSupported = isFunction(fetch);
env = utils.merge.call({
skipUndefined: true
}, globalFetchAPI, env);

const {fetch: envFetch, Request, Response} = env;
const isFetchSupported = envFetch ? isFunction(envFetch) : typeof fetch === 'function';
const isRequestSupported = isFunction(Request);
const isResponseSupported = isFunction(Response);

Expand Down Expand Up @@ -134,6 +138,8 @@ const factory = (env) => {
fetchOptions
} = resolveConfig(config);

let _fetch = envFetch || fetch;

responseType = responseType ? (responseType + '').toLowerCase() : 'text';

let composedSignal = composeSignals([signal, cancelToken && cancelToken.toAbortSignal()], timeout);
Expand Down Expand Up @@ -193,7 +199,7 @@ const factory = (env) => {

request = isRequestSupported && new Request(url, resolvedOptions);

let response = await (isRequestSupported ? fetch(request, fetchOptions) : fetch(url, resolvedOptions));
let response = await (isRequestSupported ? _fetch(request, fetchOptions) : _fetch(url, resolvedOptions));

const isStreamResponse = supportsResponseStream && (responseType === 'stream' || responseType === 'response');

Expand Down Expand Up @@ -256,12 +262,8 @@ const factory = (env) => {
const seedCache = new Map();

export const getFetch = (config) => {
let env = utils.merge.call({
skipUndefined: true
}, globalFetchAPI, config ? config.env : null);

let env = config ? config.env : {};
const {fetch, Request, Response} = env;
Comment on lines +265 to 266
Copy link

@coderabbitai coderabbitai bot Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

git ls-files lib/adapters/fetch.js

Repository: Signal65/axios-CodeRabbit

Length of output: 90

🏁 Script executed:

head -n 300 lib/adapters/fetch.js | tail -n 50

Repository: 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 js

Repository: Signal65/axios-CodeRabbit

Length of output: 246

🏁 Script executed:

rg -n 'getFetch\s*\(' --type js -A 5

Repository: Signal65/axios-CodeRabbit

Length of output: 199

🏁 Script executed:

cat lib/adapters/adapters.js

Repository: 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.


const seeds = [
Request, Response, fetch
];
Expand Down
27 changes: 27 additions & 0 deletions test/unit/adapters/fetch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,5 +489,32 @@ describe('supports fetch with nodejs', function () {

assert.strictEqual(data, 'OK');
});

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;
}
Comment on lines +493 to +517
Copy link

@coderabbitai coderabbitai bot Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the exact file and lines in question
wc -l test/unit/adapters/fetch.js

Repository: 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.js

Repository: 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 20

Repository: 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 -5

Repository: 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 10

Repository: 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 15

Repository: Signal65/axios-CodeRabbit

Length of output: 51

🏁 Script executed:

# Read the biome.json file to check noGlobalAssign configuration
cat biome.json

Repository: Signal65/axios-CodeRabbit

Length of output: 1844

🏁 Script executed:

# Also check the .eslintrc.cjs file
cat .eslintrc.cjs

Repository: 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 -A2

Repository: 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.

Suggested change
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.

});
});
});