Repository navigation
breaking change in 21.7.0 when mocking fetch #52015
Description
Activity
This is another simplified test case:
import { mock } from "node:test"; mock.method(globalThis, "fetch", async () => ({ text: async () => "foo", })); const response = await fetch("https://bar.baz/"); const text = await response.text(); console.log(text); mock.restoreAll();
@nodejs/test_runner @joyeecheung
It appears that this issue can be resolved by reintroducing the following code within the
setupUndici()function:if (!getOptionValue('--no-experimental-fetch')) { defineOperation(globalThis, 'fetch', fetch); }As a newcomer, I'm eager to contribute and learn. If there are any errors in my understanding, I would appreciate your guidance. Thank you for your assistance!
It seems to me that it’s t.mock that should be fixed, other wise it would not work with user land objects that has a similar descriptor.
I think the bug comes from the
fetchmethod, because I have the same problem with sinonjs/sinon#2590 (which doesn't usemock).The descriptor of property
fetchis different before and after using it.console.log(Object.getOwnPropertyDescriptor(globalThis, "fetch")); fetch; console.log(Object.getOwnPropertyDescriptor(globalThis, "fetch"));
-
Node.js v21.7.0:
{ get: [Function: get], set: [Function: set], enumerable: true, configurable: true } { value: [Function: fetch], writable: true, enumerable: true, configurable: true }
-
Node.js v21.6.2:
{ value: [Function: fetch], writable: true, enumerable: true, configurable: true } { value: [Function: fetch], writable: true, enumerable: true, configurable: true }
-
Chromium 122:
{ writable: true, enumerable: true, configurable: true, value: f fetch() } { writable: true, enumerable: true, configurable: true, value: f fetch() }
Reacted by Carl-Erik Kopseng, Victor Chen and koooge-
You could directly define the
fetchfunction invalue(without the intermediate steps ofgetandset). The descriptor won't change and you won't lose lazy loading.ObjectDefineProperty(globalThis, 'fetch', { __proto__: null, writable: true, value: function fetch(input, init = undefined) { // Loading undici alone lead to promises which breaks lots of tests so we // have to load it really lazily for now. const { fetch: impl } = require('internal/deps/undici/undici'); return impl(input, init); }, });
At end of the day, it seems to be a design issue with the mocking libraries, as their API forces users to know and differentiate between getters and values even though users may only care about hijacking or recording the access, but don’t care about whether the access is done via a getter or not. For example with Sinon I think you would then need to use a spy with .get(), and this applies to user objects too sinonjs/sinon#1545 That said for properties that are themselves methods, like in the case of fetch, doing the lazy loading inside the value method seems to be a good compromise if it helps users work around this issue with the mocking libraries. I am not sure about FormData and friends because as constructors wrapping them complicates instanceof, but they may be less necessary to mock anyway.
In any case, isn't it still considered a breaking change and should be in a major node version change?
There's the same problem with Node v20.12.0:
- bd2a3c1 lib: define FormData and fetch etc. in the built-in snapshot
- v20.12.0 proposal #52212
Reacted by Bruno MacedoSame problem reproduced with Node v20.12.1
Reacted by Bruno Macedo and Benito Coutinho- added a commit that references this issue
on Apr 7, 2024 - added a commit that references this issue
on Apr 29, 2024 - added 2 commits that reference this issue
on May 2, 2024
Version
21.7.0
Platform
Darwin Stevens-MacBook-Pro.local 23.2.0 Darwin Kernel Version 23.2.0: Wed Nov 15 21:54:55 PST 2023; root:xnu-10002.61.3~2/RELEASE_ARM64_T8122 arm64
Subsystem
No response
What steps will reproduce the bug?
which produces:
to fix the break, i have to reference
global.fetchbefore mocking it...ie:How often does it reproduce? Is there a required condition?
Consistently
What is the expected behavior? Why is that the expected behavior?
Should not break existing code on a minor or patch semver change of node.
What do you see instead?
It breaks
Additional information
This is the PR which introduced the break: #51598 (comment)