Closed btomaj closed 2 months ago
I've contemplated two possible solutions. The first, as described in the OP, doesn't change behaviour for existing implementations calling setAbstractFetchFunc
:
export function setAbstractFetchFunc(func?: AbstractFetchFunc) {
if (func) {
abstractFetch = func;
} else {
abstractFetch = (...args) => fetch(...args);
}
}
The second, changes the behaviour for existing implementations:
export function setAbstractFetchFunc(func: AbstractFetchFunc) {
if (typeof fetch === 'function' && func === fetch) {
abstractFetch = (...args) => fetch(...args);
} else {
abstractFetch = func;
}
}
My preference is the second solution, unless there's a part of the picture that I'm missing.
In environments where func
is not native fetch
, for example using node-fetch
, the tester will need to mock the package providing the non-native fetch
that is used when calling setAbstractFetchFunc
.
This is an interesting one. You're right that we added the abstract fetch function back when it wasn't supported on node, but we're only supporting 18+ now, so it should be possible to fully drop it (AFAIK node 18 introduced native support) - we're even setting it back to the native call for vercel environments.
We could consider simply using the native implementation for node as well if that doesn't break things for node 18, which would mean the only override would be for our tests. That should solve the problem.
One thing to note is that you can simply call this in your app:
// Sets to node-fetch
import "@shopify/shopify-api/adapters/node";
// Set it back
setAbstractFetchFunc(fetch);
and set it back to the native definition, so you can work around the problem for now. This function doesn't have any side effects so it can be called multiple times.
One thing to note is that you can simply call this in your app: ...
setAbstractFetchFunc(fetch);
...so you can work around the problem for now. This function doesn't have any side effects so it can be called multiple times.
Unfortunately this doesn't work when testing Remix apps that import @shopify/shopify-app-remix
because @shopify/shopify-api/adapters/web-api
is imported in @shopify/shopify-app-remix
. Can I submit a PR for the second proposed solution as an interim minor change until the issue of abstractFetch
is resolved? Assessing the impact of unwinding abstractFetch
both within the codebase and on app developers might not be trivial.
Unfortunately this doesn't work when testing Remix apps that import @shopify/shopify-app-remix because @shopify/shopify-api/adapters/web-api is imported in @shopify/shopify-app-remix
True. I think it should still work if you do this:
import "@shopify/shopify-app-remix/adapters/node";
import { setAbstractFetchFunc } from "@shopify/shopify-api/runtime";
setAbstractFetchFunc(fetch);
since shopify-api
is an implied dependency and the remix package just calls through, but it starts to get too complicated, which probably means it's not a good solution :)
I think as an interim solution, the second proposal is OK. I don't think it would break any existing apps - if they were already setting it to fetch
, they'd continue to be using fetch
in the end, right? Or am I missing something?
Nit: I'd consider just setting abstractFetchFun = fetch
in that case too, if it works, instead of aliasing it.
Unfortunately this doesn't work when testing Remix apps that import @shopify/shopify-app-remix because @shopify/shopify-api/adapters/web-api is imported in @shopify/shopify-app-remix
True. I think it should still work if you do this:
import "@shopify/shopify-app-remix/adapters/node"; import { setAbstractFetchFunc } from "@shopify/shopify-api/runtime"; setAbstractFetchFunc(fetch); since
shopify-api
is an implied dependency and the remix package just calls through, but it starts to get too complicated, which probably means it's not a good solution :)
You were right! setAbstractFetchFun(fetch)
solved the problem. The issue turned out to be with my testing framework and import
statements in code under test being executed before beforeAll()
.
Thank you 🙌
I've run into an issue mocking REST and GraphQL queries using MSW in testing because
setAbstractFetchFunc
stores a reference tofetch
, such that if fetch is overwritten at runtime,abstractFetch
continues to point to the originalfetch
reference.One possible solution is to make
func
an optional parameter, and storing a reference againstabstractFetch
only if it's provided throughfunc
, or callingfetch
(equal toglobalThis.fetch
) directly if not, such that it calls the overwrittenfetch
when it is overwritten.What are your thoughts? My working assumption is that
abstractFetch
is still required for node.js backward compatibility.