diff --git a/.changeset/remove-node-fetch-from-cli-kit.md b/.changeset/remove-node-fetch-from-cli-kit.md new file mode 100644 index 00000000000..2b4d82ddc08 --- /dev/null +++ b/.changeset/remove-node-fetch-from-cli-kit.md @@ -0,0 +1,5 @@ +--- +'@shopify/cli-kit': patch +--- + +Replace node-fetch and form-data with the standard fetch implementation; `@shopify/cli-kit/node/http` no longer exports `FetchError` diff --git a/packages/app/package.json b/packages/app/package.json index 48b89d3965b..f5cfb95cc30 100644 --- a/packages/app/package.json +++ b/packages/app/package.json @@ -83,7 +83,8 @@ "@types/react": "^19.0.0", "@types/which": "3.0.4", "@types/ws": "^8.5.13", - "@vitest/coverage-istanbul": "^3.2.7" + "@vitest/coverage-istanbul": "^3.2.7", + "undici": "8.10.0" }, "engines": { "node": ">=22.12.0" diff --git a/packages/app/src/cli/services/bundle.ts b/packages/app/src/cli/services/bundle.ts index ec0f0b9d20b..6bef1984478 100644 --- a/packages/app/src/cli/services/bundle.ts +++ b/packages/app/src/cli/services/bundle.ts @@ -65,13 +65,13 @@ export async function uploadToGCS(signedURL: string, filePath: string) { let response: Response | undefined for (let attempt = 1; attempt <= UPLOAD_MAX_ATTEMPTS; attempt++) { // The signed URL only signs the `host` header, so no extra headers are - // required; node-fetch derives Content-Length from the buffer body. + // required; fetch derives Content-Length from the buffer body. // eslint-disable-next-line no-await-in-loop response = await fetch(signedURL, {method: 'put', body: buffer}, 'slow-request') if (response.ok) return const lastAttempt = attempt === UPLOAD_MAX_ATTEMPTS const retryable = RETRYABLE_UPLOAD_STATUS_CODES.has(response.status) - // node-fetch keeps the socket open until the body is consumed. On the final + // fetch keeps the socket open until the body is consumed. On the final // attempt we read it below for the error message; otherwise drain it here so // the connection can be reused or released before the next attempt. if (retryable && !lastAttempt) { diff --git a/packages/app/src/cli/services/dev/processes/dev-session/dev-session-process.test.ts b/packages/app/src/cli/services/dev/processes/dev-session/dev-session-process.test.ts index f8422e5e7de..2f817c0b0ab 100644 --- a/packages/app/src/cli/services/dev/processes/dev-session/dev-session-process.test.ts +++ b/packages/app/src/cli/services/dev/processes/dev-session/dev-session-process.test.ts @@ -25,7 +25,6 @@ vi.mock('@shopify/cli-kit/node/fs') vi.mock('@shopify/cli-kit/node/archiver') vi.mock('@shopify/cli-kit/node/http') vi.mock('../../../../utilities/app/app-url.js') -vi.mock('node-fetch') vi.mock('../../../bundle.js') describe('setupDevSessionProcess', () => { test('returns a dev session process with correct configuration', async () => { diff --git a/packages/app/src/cli/services/webhook/send-app-uninstalled-webhook.test.ts b/packages/app/src/cli/services/webhook/send-app-uninstalled-webhook.test.ts index 025c8ac52dd..120adac2aeb 100644 --- a/packages/app/src/cli/services/webhook/send-app-uninstalled-webhook.test.ts +++ b/packages/app/src/cli/services/webhook/send-app-uninstalled-webhook.test.ts @@ -2,7 +2,6 @@ import {sendUninstallWebhookToAppServer} from './send-app-uninstalled-webhook.js import {triggerLocalWebhook} from './trigger-local-webhook.js' import {testDeveloperPlatformClient} from '../../models/app/app.test-data.js' import {describe, expect, vi, test} from 'vitest' -import {FetchError} from '@shopify/cli-kit/node/http' import {Writable} from 'stream' vi.mock('./trigger-local-webhook.js') @@ -59,8 +58,9 @@ describe('sendUninstallWebhookToAppServer', () => { }) test("retries the webhook request if the app hasn't started yet", async () => { - const fakeError = new FetchError('Fake error for testing', 'network') - fakeError.code = 'ECONNREFUSED' + const fakeError = Object.assign(new TypeError('fetch failed'), { + cause: Object.assign(new Error('connect ECONNREFUSED'), {code: 'ECONNREFUSED'}), + }) vi.mocked(triggerLocalWebhook).mockRejectedValueOnce(fakeError).mockResolvedValueOnce(true) const stdout = {write: vi.fn()} as unknown as Writable const developerPlatformClient = testDeveloperPlatformClient() diff --git a/packages/app/src/cli/services/webhook/send-app-uninstalled-webhook.ts b/packages/app/src/cli/services/webhook/send-app-uninstalled-webhook.ts index 95ed3f37610..91499f440d1 100644 --- a/packages/app/src/cli/services/webhook/send-app-uninstalled-webhook.ts +++ b/packages/app/src/cli/services/webhook/send-app-uninstalled-webhook.ts @@ -3,10 +3,21 @@ import {getWebhookSample, SampleWebhook, SendSampleWebhookVariables} from './req import {triggerLocalWebhook} from './trigger-local-webhook.js' import {DELIVERY_METHOD} from './trigger-flags.js' import {DeveloperPlatformClient} from '../../utilities/developer-platform-client.js' -import {FetchError} from '@shopify/cli-kit/node/http' import {sleep} from '@shopify/cli-kit/node/system' import {Writable} from 'stream' +// fetch wraps connection failures in a TypeError whose cause carries the +// system error code, possibly inside an AggregateError when several +// addresses were tried (e.g. IPv4 and IPv6 localhost). +function isConnectionRefusedError(error: unknown): boolean { + if (!(error instanceof Error)) return false + const cause = (error as {cause?: unknown}).cause + if (!cause || typeof cause !== 'object') return false + const causeWithCode = cause as {code?: string; errors?: {code?: string}[]} + if (causeWithCode.code === 'ECONNREFUSED') return true + return Array.isArray(causeWithCode.errors) && causeWithCode.errors.some((err) => err.code === 'ECONNREFUSED') +} + interface SendUninstallWebhookToAppServerOptions { stdout: Writable developerPlatformClient: DeveloperPlatformClient @@ -60,7 +71,7 @@ async function triggerWebhook( return result } catch (error) { - if (error instanceof FetchError && error.code === 'ECONNREFUSED') { + if (isConnectionRefusedError(error)) { if (tries < 3) { options.stdout.write("App isn't responding yet, retrying in 5 seconds") await sleep(5) diff --git a/packages/app/src/cli/utilities/app/http-reverse-proxy.test.ts b/packages/app/src/cli/utilities/app/http-reverse-proxy.test.ts index 7d26a2bf0c8..6018cfab7e0 100644 --- a/packages/app/src/cli/utilities/app/http-reverse-proxy.test.ts +++ b/packages/app/src/cli/utilities/app/http-reverse-proxy.test.ts @@ -1,7 +1,7 @@ import {getProxyingWebServer} from './http-reverse-proxy.js' import {AbortController} from '@shopify/cli-kit/node/abort' import {describe, test, expect} from 'vitest' -import fetch from 'node-fetch' +import {fetch, Agent} from 'undici' import WebSocket, {WebSocketServer} from 'ws' import http from 'http' import https from 'https' @@ -16,17 +16,19 @@ describe.sequential.each(each)('http-reverse-proxy for %s', (protocol) => { protocol === 'http' ? new http.Agent({keepAlive: false}) : new https.Agent({ca: localhostCert.cert, keepAlive: false}) + const dispatcher = + protocol === 'http' ? new Agent({pipelining: 0}) : new Agent({pipelining: 0, connect: {ca: localhostCert.cert}}) test('routes requests to the correct target based on path', {retry: 2}, async ({setup}) => { - const response1 = await fetch(`${protocol}://localhost:${setup.proxyPort}/path1/test`, {agent}) + const response1 = await fetch(`${protocol}://localhost:${setup.proxyPort}/path1/test`, {dispatcher}) await expect(response1.text()).resolves.toBe('Response from target server 1') - const response2 = await fetch(`${protocol}://localhost:${setup.proxyPort}/path2/test`, {agent}) + const response2 = await fetch(`${protocol}://localhost:${setup.proxyPort}/path2/test`, {dispatcher}) await expect(response2.text()).resolves.toBe('Response from target server 2') }) test('routes requests to the default target when no matching path is found', {retry: 2}, async ({setup}) => { - const response = await fetch(`${protocol}://localhost:${setup.proxyPort}/unknown/path`, {agent}) + const response = await fetch(`${protocol}://localhost:${setup.proxyPort}/unknown/path`, {dispatcher}) await expect(response.text()).resolves.toBe('Response from target server 1') }) @@ -63,7 +65,7 @@ describe.sequential.each(each)('http-reverse-proxy for %s', (protocol) => { 'Access-Control-Request-Method': 'GET', 'Access-Control-Request-Headers': 'Authorization', }, - agent, + dispatcher, }) expect(response.status).toBe(204) expect(response.headers.get('access-control-allow-origin')).toBe('https://extensions.shopifycdn.com') @@ -75,7 +77,7 @@ describe.sequential.each(each)('http-reverse-proxy for %s', (protocol) => { test('responds to CORS preflight OPTIONS with defaults when no request headers', {retry: 2}, async ({setup}) => { const response = await fetch(`${protocol}://localhost:${setup.proxyPort}/path1/test`, { method: 'OPTIONS', - agent, + dispatcher, }) expect(response.status).toBe(204) expect(response.headers.get('access-control-allow-origin')).toBe('*') @@ -89,7 +91,7 @@ describe.sequential.each(each)('http-reverse-proxy for %s', (protocol) => { await expect .poll(async () => { try { - await fetch(`${protocol}://localhost:${setup.proxyPort}/path1`, {agent}) + await fetch(`${protocol}://localhost:${setup.proxyPort}/path1`, {dispatcher}) return 'open' // eslint-disable-next-line no-catch-all/no-catch-all } catch { diff --git a/packages/cli-kit/package.json b/packages/cli-kit/package.json index 37600097d58..436af2b7523 100644 --- a/packages/cli-kit/package.json +++ b/packages/cli-kit/package.json @@ -130,7 +130,6 @@ "fast-glob": "3.3.3", "figures": "5.0.0", "find-up": "6.3.0", - "form-data": "4.0.6", "fs-extra": "11.1.0", "gradient-string": "2.0.2", "graphql": "16.14.2", @@ -147,7 +146,6 @@ "minimatch": "9.0.9", "mrmime": "1.0.1", "network-interfaces": "1.1.0", - "node-fetch": "3.3.2", "open": "8.4.2", "pathe": "1.1.2", "react": "19.2.4", @@ -156,6 +154,7 @@ "stacktracey": "2.2.0", "strip-ansi": "7.2.0", "supports-hyperlinks": "3.2.0", + "undici": "8.10.0", "which": "4.0.0", "zod": "3.25.76" }, diff --git a/packages/cli-kit/src/private/node/api.ts b/packages/cli-kit/src/private/node/api.ts index 044dd0abfb2..13920e3e7ec 100644 --- a/packages/cli-kit/src/private/node/api.ts +++ b/packages/cli-kit/src/private/node/api.ts @@ -4,11 +4,14 @@ import {sleepWithBackoffUntil} from './sleep-with-backoff.js' import {outputDebug} from '../../public/node/output.js' import {recordRetry} from '../../public/node/analytics.js' -import {Headers} from 'form-data' import {ClientError} from 'graphql-request' import {performance} from 'perf_hooks' +interface Headers { + forEach(callbackfn: (value: string, key: string) => void): void +} + export type API = 'admin' | 'storefront-renderer' | 'partners' | 'business-platform' | 'app-management' export const allAPIs: API[] = ['admin', 'storefront-renderer', 'partners', 'business-platform', 'app-management'] diff --git a/packages/cli-kit/src/private/node/session/device-authorization.test.ts b/packages/cli-kit/src/private/node/session/device-authorization.test.ts index e87ab4fc703..f093fc46bfc 100644 --- a/packages/cli-kit/src/private/node/session/device-authorization.test.ts +++ b/packages/cli-kit/src/private/node/session/device-authorization.test.ts @@ -15,7 +15,7 @@ import {isCI, openURL} from '../../../public/node/system.js' import * as output from '../../../public/node/output.js' import {beforeEach, describe, expect, test, vi} from 'vitest' -import {Response} from 'node-fetch' +import {Response} from 'undici' vi.mock('../../../public/node/context/fqdn.js') vi.mock('./identity') @@ -151,7 +151,7 @@ describe('requestDeviceAuthorization', () => { Object.defineProperty(response, 'status', {value: 200}) Object.defineProperty(response, 'statusText', {value: 'OK'}) // Mock text() to throw an error - response.text = vi.fn().mockRejectedValue(new Error('Network error')) + Object.defineProperty(response, 'text', {value: vi.fn().mockRejectedValue(new Error('Network error'))}) vi.mocked(shopifyFetch).mockResolvedValue(response) vi.mocked(identityFqdn).mockResolvedValue('fqdn.com') vi.mocked(clientId).mockReturnValue('clientId') diff --git a/packages/cli-kit/src/private/node/session/device-authorization.ts b/packages/cli-kit/src/private/node/session/device-authorization.ts index 8ac629e7cc0..57ed364b9ae 100644 --- a/packages/cli-kit/src/private/node/session/device-authorization.ts +++ b/packages/cli-kit/src/private/node/session/device-authorization.ts @@ -2,13 +2,11 @@ import {clientId} from './identity.js' import {exchangeDeviceCodeForAccessToken} from './exchange.js' import {IdentityToken} from './schema.js' import {identityFqdn} from '../../../public/node/context/fqdn.js' -import {shopifyFetch} from '../../../public/node/http.js' +import {shopifyFetch, Response} from '../../../public/node/http.js' import {outputContent, outputDebug, outputInfo, outputToken} from '../../../public/node/output.js' import {AbortError, BugError} from '../../../public/node/error.js' import {isCI, openURL} from '../../../public/node/system.js' -import {Response} from 'node-fetch' - export interface DeviceAuthorizationResponse { deviceCode: string userCode: string diff --git a/packages/cli-kit/src/private/node/session/exchange.test.ts b/packages/cli-kit/src/private/node/session/exchange.test.ts index 8c1b576b36f..bc4c9aadab6 100644 --- a/packages/cli-kit/src/private/node/session/exchange.test.ts +++ b/packages/cli-kit/src/private/node/session/exchange.test.ts @@ -19,7 +19,7 @@ import {outputDebug} from '../../../public/node/output.js' import {err, ok} from '../../../public/node/result.js' import {describe, test, expect, vi, afterAll, beforeEach} from 'vitest' -import {Response} from 'node-fetch' +import {Response} from 'undici' const currentDate = new Date(2022, 1, 1, 10) const expiredDate = new Date(2022, 1, 1, 11) diff --git a/packages/cli-kit/src/public/node/api/admin.test.ts b/packages/cli-kit/src/public/node/api/admin.test.ts index 1a503448dfd..3346ef2f54a 100644 --- a/packages/cli-kit/src/public/node/api/admin.test.ts +++ b/packages/cli-kit/src/public/node/api/admin.test.ts @@ -119,12 +119,12 @@ describe('admin-rest-api', () => { // Given const json = () => Promise.resolve({result: true}) const status = 200 - const headers = {'some-header': 123} + const headers = {'some-header': '123'} vi.spyOn(http, 'shopifyFetch').mockResolvedValue({ json, status, - headers: {raw: () => headers}, + headers: new Headers(headers), } as any) // When @@ -133,7 +133,7 @@ describe('admin-rest-api', () => { // Then expect(result.json).toEqual({result: true}) expect(result.status).toEqual(200) - expect(result.headers).toEqual({'some-header': 123}) + expect(result.headers).toEqual({'some-header': ['123']}) }) test('fetch is called with correct parameters', async () => { @@ -146,7 +146,7 @@ describe('admin-rest-api', () => { const spyFetch = vi.spyOn(http, 'shopifyFetch').mockResolvedValue({ json, status, - headers: {raw: () => ({})}, + headers: new Headers(), } as any) // When @@ -173,7 +173,7 @@ describe('admin-rest-api', () => { const spyFetch = vi.spyOn(http, 'shopifyFetch').mockResolvedValue({ json: () => Promise.resolve({result: true}), status, - headers: {raw: () => ({})}, + headers: new Headers(), } as any) // When diff --git a/packages/cli-kit/src/public/node/api/admin.ts b/packages/cli-kit/src/public/node/api/admin.ts index f496b9d533b..98c1e7cf92c 100644 --- a/packages/cli-kit/src/public/node/api/admin.ts +++ b/packages/cli-kit/src/public/node/api/admin.ts @@ -267,10 +267,15 @@ export async function restRequest( const json = await response.json().catch(() => ({})) + const responseHeaders: Record = {} + response.headers.forEach((value, key) => { + responseHeaders[key] = key.toLowerCase() === 'set-cookie' ? response.headers.getSetCookie() : [value] + }) + return { json, status: response.status, - headers: response.headers.raw(), + headers: responseHeaders, } } diff --git a/packages/cli-kit/src/public/node/api/bulk-operations/stage-file.test.ts b/packages/cli-kit/src/public/node/api/bulk-operations/stage-file.test.ts index 70515518d3e..44b08ed9ceb 100644 --- a/packages/cli-kit/src/public/node/api/bulk-operations/stage-file.test.ts +++ b/packages/cli-kit/src/public/node/api/bulk-operations/stage-file.test.ts @@ -1,8 +1,9 @@ import {stageFile} from './stage-file.js' import {adminRequestDoc} from '../admin.js' -import {fetch} from '../../http.js' +import {fetch, formData} from '../../http.js' import {renderSingleTask, RenderSingleTaskOptions} from '../../ui.js' import {describe, test, expect, vi, beforeEach} from 'vitest' +import {FormData} from 'undici' vi.mock('../admin.js') vi.mock('../../session.js') @@ -37,6 +38,7 @@ describe('stageFile', () => { } beforeEach(() => { + vi.mocked(formData).mockImplementation(() => new FormData()) vi.mocked(renderSingleTask).mockImplementation(async (options: RenderSingleTaskOptions) => { return options.task(vi.fn()) }) diff --git a/packages/cli-kit/src/public/node/api/bulk-operations/stage-file.ts b/packages/cli-kit/src/public/node/api/bulk-operations/stage-file.ts index 50fedd55a7e..e0f3a694f6e 100644 --- a/packages/cli-kit/src/public/node/api/bulk-operations/stage-file.ts +++ b/packages/cli-kit/src/public/node/api/bulk-operations/stage-file.ts @@ -5,7 +5,7 @@ import { } from '../../../../cli/api/graphql/bulk-operations/generated/staged-uploads-create.js' import {adminRequestDoc} from '../admin.js' import {AdminSession} from '../../session.js' -import {fetch} from '../../http.js' +import {fetch, formData} from '../../http.js' import {AbortError} from '../../error.js' import {outputContent} from '../../output.js' import {renderSingleTask} from '../../ui.js' @@ -103,7 +103,7 @@ async function uploadFileToStagedUrl( parameters: {name: string; value: string}[], filename: string, ): Promise { - const form = new FormData() + const form = formData() for (const param of parameters) { form.append(param.name, param.value) diff --git a/packages/cli-kit/src/public/node/github.test.ts b/packages/cli-kit/src/public/node/github.test.ts index d02f427c6e4..4dc2f8a6c67 100644 --- a/packages/cli-kit/src/public/node/github.test.ts +++ b/packages/cli-kit/src/public/node/github.test.ts @@ -12,7 +12,7 @@ import {joinPath} from './path.js' import {readFile} from './fs.js' import {isExecutable} from 'is-executable' import {describe, expect, test, vi} from 'vitest' -import {Response} from 'node-fetch' +import {Response} from 'undici' vi.mock('./http.js') diff --git a/packages/cli-kit/src/public/node/http.test.ts b/packages/cli-kit/src/public/node/http.test.ts index f848c4e76a6..d3ebebb4919 100644 --- a/packages/cli-kit/src/public/node/http.test.ts +++ b/packages/cli-kit/src/public/node/http.test.ts @@ -4,67 +4,48 @@ import {fileExists, inTemporaryDirectory, readFile} from './fs.js' import {joinPath} from './path.js' import {getAllPublicMetadata} from './metadata.js' import {platformAndArch} from './os.js' -import {afterAll, afterEach, beforeAll, beforeEach, describe, expect, test, vi} from 'vitest' -import {setupServer} from 'msw/node' -import {delay, http, HttpResponse} from 'msw' -import FormData from 'form-data' +import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest' +import {FormData, MockAgent, getGlobalDispatcher, setGlobalDispatcher} from 'undici' +import type {Dispatcher} from 'undici' const DURATION_UNTIL_ABORT_IS_SEEN = 100 +const NEVER_RESPONDS_DELAY_MS = 10 * 60 * 1000 const mockResponse = {hello: 'world!'} -const handlers = [ - http.get('https://shopify.example/working', () => { - return HttpResponse.json(mockResponse) - }), - http.get('https://shopify.example/a-slow-endpoint', async () => { - await delay(500) - return HttpResponse.json(mockResponse) - }), - http.get('https://shopify.example/a-blocked-endpoint', async () => { - await delay('infinite') - return HttpResponse.json(mockResponse) - }), - http.get('https://shopify.example/example.txt', async () => { - const stream = new ReadableStream({ - start(controller) { - const encoder = new TextEncoder() - controller.enqueue(encoder.encode('Hello ')) - controller.enqueue(encoder.encode('world')) - controller.close() - }, - }) - return new HttpResponse(stream, { - headers: { - 'Content-Type': 'text/plain', - }, - }) - }), - http.get('https://shopify.example/fails-to-download.txt', async () => { - return HttpResponse.error() - }), - http.get('https://shopify.example/redirect-me', () => { - return new HttpResponse(null, { - status: 302, - headers: { - Location: 'https://shopify.example/example.txt', - }, - }) - }), - http.get('https://shopify.example/500.txt', () => { - return new HttpResponse(null, { - status: 500, - }) - }), -] - -// set up the server & clean-up -const server = setupServer(...handlers) -beforeAll(() => server.listen({onUnhandledRequest: 'error'})) -afterAll(() => server.close()) -afterEach(() => { - server.resetHandlers() - server.events.removeAllListeners() +function setUpMockServer(): MockAgent { + const agent = new MockAgent({enableCallHistory: true}) + agent.disableNetConnect() + const pool = agent.get('https://shopify.example') + pool + .intercept({path: /^\/working/}) + .reply(200, mockResponse) + .persist() + pool.intercept({path: '/a-slow-endpoint'}).reply(200, mockResponse).delay(500).persist() + pool.intercept({path: '/a-blocked-endpoint'}).reply(200, mockResponse).delay(NEVER_RESPONDS_DELAY_MS).persist() + pool + .intercept({path: /^\/example\.txt/}) + .reply(200, 'Hello world', {headers: {'Content-Type': 'text/plain'}}) + .persist() + pool.intercept({path: '/fails-to-download.txt'}).replyWithError(new Error('Network error')).persist() + pool + .intercept({path: '/redirect-me'}) + .reply(302, '', {headers: {Location: 'https://shopify.example/example.txt'}}) + .persist() + return agent +} + +// set up the mock server & clean-up +let mockAgent: MockAgent +let originalDispatcher: Dispatcher +beforeEach(() => { + originalDispatcher = getGlobalDispatcher() + mockAgent = setUpMockServer() + setGlobalDispatcher(mockAgent) +}) +afterEach(async () => { + setGlobalDispatcher(originalDispatcher) + await mockAgent.close() }) // set-up fake timers & clean-up @@ -80,7 +61,7 @@ describe('formData', () => { test('make an empty form data object', () => { const res = formData() expect(res).toBeInstanceOf(FormData) - expect(res.getLengthSync()).toBe(0) + expect([...res.entries()]).toHaveLength(0) }) }) @@ -113,7 +94,7 @@ describe('shopifyFetch', () => { }) await vi.advanceTimersByTimeAsync(DURATION_UNTIL_ABORT_IS_SEEN) - await expect(failing).rejects.toThrow('The operation was aborted.') + await expect(failing).rejects.toThrow(/aborted/) }) test('abort signal is seen as a retryable error', async () => { @@ -121,11 +102,6 @@ describe('shopifyFetch', () => { // all competing to run at the same time. so: we use real timers and some adjusted limits -- the test takes 500ms. vi.useRealTimers() - const requests: string[] = [] - server.events.on('request:start', ({request}) => { - requests.push(request.url) - }) - // the limit is 1100ms, which is enough for two retries plus maximum slack (e.g. running in a slow environment) const failingWithRetry = shopifyFetch(`https://shopify.example/a-blocked-endpoint`, undefined, { useNetworkLevelRetry: true, @@ -133,7 +109,7 @@ describe('shopifyFetch', () => { useAbortSignal: true, timeoutMs: DURATION_UNTIL_ABORT_IS_SEEN, }) - await expect(failingWithRetry).rejects.toThrow('The operation was aborted.') + await expect(failingWithRetry).rejects.toThrow(/aborted/) // we have enough time for two requests in our 500ms allowance: // - we make a request @@ -143,6 +119,7 @@ describe('shopifyFetch', () => { // - there's 100ms before the abort signal is seen // - the next delay would be 600ms // - the next delay would take us over our 1100ms allowance, so retries are stopped + const requests = (mockAgent.getCallHistory()?.calls() ?? []).map((call) => call.fullUrl) expect(requests).toEqual([ 'https://shopify.example/a-blocked-endpoint', 'https://shopify.example/a-blocked-endpoint', @@ -157,7 +134,7 @@ describe('shopifyFetch', () => { }) await vi.advanceTimersByTimeAsync(DURATION_UNTIL_ABORT_IS_SEEN) - await expect(response).rejects.toThrow('The operation was aborted.') + await expect(response).rejects.toThrow(/aborted/) }) test('provide a hard-coded abort signal', async () => { @@ -168,7 +145,7 @@ describe('shopifyFetch', () => { }) await vi.advanceTimersByTimeAsync(DURATION_UNTIL_ABORT_IS_SEEN) - await expect(response).rejects.toThrow('The operation was aborted.') + await expect(response).rejects.toThrow(/aborted/) }) test('provide an abort signal through request init option', async () => { @@ -185,7 +162,7 @@ describe('shopifyFetch', () => { ) await vi.advanceTimersByTimeAsync(DURATION_UNTIL_ABORT_IS_SEEN) - await expect(response).rejects.toThrow('The operation was aborted.') + await expect(response).rejects.toThrow(/aborted/) }) }) @@ -288,7 +265,7 @@ describe('downloadFile', () => { // When const result = downloadFile(url, to) - await expect(result).rejects.toThrow('Network error') + await expect(result).rejects.toThrow('fetch failed') await expect(fileExists(to)).resolves.toBe(false) }) }) diff --git a/packages/cli-kit/src/public/node/http.ts b/packages/cli-kit/src/public/node/http.ts index 86bda5a9f9b..606c9132caf 100644 --- a/packages/cli-kit/src/public/node/http.ts +++ b/packages/cli-kit/src/public/node/http.ts @@ -4,15 +4,15 @@ import {runWithTimer} from './metadata.js' import {maxRequestTimeForNetworkCallsMs, skipNetworkLevelRetry} from './environment.js' import {outputContent, outputDebug, outputToken} from './output.js' import {sanitizeURL} from '../../private/node/api/urls.js' -import {httpsAgent, sanitizedHeadersOutput} from '../../private/node/api/headers.js' +import {sanitizedHeadersOutput} from '../../private/node/api/headers.js' import {NetworkRetryBehaviour, simpleRequestWithDebugLog} from '../../private/node/api.js' import {DEFAULT_MAX_TIME_MS} from '../../private/node/sleep-with-backoff.js' -import FormData from 'form-data' -import nodeFetch, {RequestInfo, RequestInit, Response} from 'node-fetch' +import {fetch as undiciFetch, EnvHttpProxyAgent, FormData, Response} from 'undici' import {pipeline} from 'stream/promises' +import type {Dispatcher, RequestInfo, RequestInit} from 'undici' -export {FetchError, Request, Response} from 'node-fetch' +export {FormData, Request, Response} from 'undici' /** * Create a new FormData object. @@ -96,7 +96,30 @@ interface FetchOptions { behaviour: RequestBehaviour init?: RequestInit logRequest: boolean - useHttpsAgent: boolean +} + +let proxyDispatcher: Dispatcher | undefined +let proxyDispatcherComputed = false + +/** + * Returns a dispatcher that routes requests through the proxy configured with the + * SHOPIFY_HTTP_PROXY, SHOPIFY_HTTPS_PROXY and SHOPIFY_NO_PROXY environment variables, + * or undefined when no proxy is configured. These are the same variables that + * global-agent honors for the http traffic that goes through Node's http module. + * + * @param env - Process environment variables. + * @returns A dispatcher, or undefined when no proxy is configured. + */ +function dispatcherFromEnvironment(env: NodeJS.ProcessEnv = process.env): Dispatcher | undefined { + if (!proxyDispatcherComputed) { + proxyDispatcherComputed = true + const httpProxy = env.SHOPIFY_HTTP_PROXY + const httpsProxy = env.SHOPIFY_HTTPS_PROXY ?? httpProxy + if (httpProxy ?? httpsProxy) { + proxyDispatcher = new EnvHttpProxyAgent({httpProxy, httpsProxy, noProxy: env.SHOPIFY_NO_PROXY}) + } + } + return proxyDispatcher } /** @@ -117,7 +140,7 @@ export function abortSignalFromRequestBehaviour(behaviour: RequestBehaviour): Ab return signal } -async function innerFetch({url, behaviour, init, logRequest, useHttpsAgent}: FetchOptions): Promise { +async function innerFetch({url, behaviour, init, logRequest}: FetchOptions): Promise { if (logRequest) { outputDebug(outputContent`Sending ${init?.method ?? 'GET'} request to URL ${sanitizeURL(url.toString())} With request headers: @@ -125,10 +148,7 @@ ${sanitizedHeadersOutput((init?.headers ?? {}) as Record)} `) } - let agent: RequestInit['agent'] - if (useHttpsAgent) { - agent = await httpsAgent() - } + const dispatcher = init?.dispatcher ?? dispatcherFromEnvironment() const request = async () => { // each time we make the request, we need to potentially reset the abort signal, as the request logic may make @@ -140,7 +160,7 @@ ${sanitizedHeadersOutput((init?.headers ?? {}) as Record)} signal = init.signal } - return nodeFetch(url, {...init, agent, signal}) + return undiciFetch(url, {...init, dispatcher, signal}) } return runWithTimer('cmd_all_timing_network_ms')(async () => { @@ -153,12 +173,8 @@ ${sanitizedHeadersOutput((init?.headers ?? {}) as Record)} } /** - * An interface that abstracts way node-fetch. When Node has built-in - * support for "fetch" in the standard library, we can drop the node-fetch - * dependency from here. - * Note that we are exposing types from "node-fetch". The reason being is that - * they are consistent with the Web API so if we drop node-fetch in the future - * it won't require changes from the callers. + * An interface that abstracts away the fetch implementation (undici). The exposed + * types are consistent with the Web API. * * The CLI's fetch function supports special behaviours, like automatic retries. These are disabled by default through * this function. @@ -177,7 +193,6 @@ export async function fetch( url, init, logRequest: false, - useHttpsAgent: false, // all special behaviours are disabled by default behaviour: preferredBehaviour ? requestMode(preferredBehaviour) : requestMode('non-blocking'), } as const @@ -206,7 +221,6 @@ export async function shopifyFetch( url, init, logRequest: true, - useHttpsAgent: true, // special behaviours enabled by default behaviour: preferredBehaviour ? requestMode(preferredBehaviour) : requestMode(), } @@ -243,7 +257,7 @@ export function downloadFile(url: string, to: string): Promise { } try { - const res = await nodeFetch(url, {redirect: 'follow'}) + const res = await undiciFetch(url, {redirect: 'follow', dispatcher: dispatcherFromEnvironment()}) if (!res.body) { throw new Error(`No response body received when downloading ${sanitizedUrl}`) } diff --git a/packages/cli/src/cli/repo-health.test.ts b/packages/cli/src/cli/repo-health.test.ts index 45042859bf7..4ebb26b707a 100644 --- a/packages/cli/src/cli/repo-health.test.ts +++ b/packages/cli/src/cli/repo-health.test.ts @@ -52,7 +52,6 @@ describe('Node dependency version sync', () => { 'graphql-tag', 'ink', 'liquidjs', - 'node-fetch', 'typescript', 'vite', 'vitest', diff --git a/packages/plugin-cloudflare/package.json b/packages/plugin-cloudflare/package.json index 36448086caa..b5d70609637 100644 --- a/packages/plugin-cloudflare/package.json +++ b/packages/plugin-cloudflare/package.json @@ -49,7 +49,8 @@ "@shopify/cli-kit": "4.6.0" }, "devDependencies": { - "@vitest/coverage-istanbul": "^3.2.7" + "@vitest/coverage-istanbul": "^3.2.7", + "undici": "8.10.0" }, "engines": { "node": ">=22.12.0" diff --git a/packages/plugin-cloudflare/src/install-cloudflared.test.ts b/packages/plugin-cloudflare/src/install-cloudflared.test.ts index e29f90669bf..f580ce95d9f 100644 --- a/packages/plugin-cloudflare/src/install-cloudflared.test.ts +++ b/packages/plugin-cloudflare/src/install-cloudflared.test.ts @@ -3,7 +3,7 @@ import * as http from '@shopify/cli-kit/node/http' import {inTemporaryDirectory, readFile, writeFile, fileExists} from '@shopify/cli-kit/node/fs' import {joinPath} from '@shopify/cli-kit/node/path' import {describe, expect, test, vi} from 'vitest' -import {Response} from 'node-fetch' +import {Response} from 'undici' import {writeFileSync} from 'fs' // eslint-disable-next-line no-restricted-imports import * as childProcess from 'child_process' diff --git a/packages/theme/src/cli/utilities/theme-environment/storefront-session.test.ts b/packages/theme/src/cli/utilities/theme-environment/storefront-session.test.ts index a40f0b3395e..ecb4cd6f1a9 100644 --- a/packages/theme/src/cli/utilities/theme-environment/storefront-session.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/storefront-session.test.ts @@ -393,7 +393,7 @@ describe('Storefront API', () => { }) // Tests rely on this function because the 'packages/theme' package cannot - // directly access node-fetch and they use: new Response('OK', {status: 200}) + // directly access undici and they use: new Response('OK', {status: 200}) function response(mock: { status: number url?: string @@ -407,7 +407,7 @@ describe('Storefront API', () => { ...mock, headers: { ...mock.headers, - raw: vi.fn().mockReturnValue({'set-cookie': setCookieArray}), + getSetCookie: vi.fn().mockReturnValue(setCookieArray), get: vi.fn().mockImplementation((key) => mock.headers?.[key]), }, } as any diff --git a/packages/theme/src/cli/utilities/theme-environment/storefront-session.ts b/packages/theme/src/cli/utilities/theme-environment/storefront-session.ts index e6030f0d881..da3b1250f7f 100644 --- a/packages/theme/src/cli/utilities/theme-environment/storefront-session.ts +++ b/packages/theme/src/cli/utilities/theme-environment/storefront-session.ts @@ -113,7 +113,7 @@ async function sessionEssentialCookie(storeUrl: string, themeId: string, headers headers: requestHeaders, }) - const setCookies = response.headers.raw()['set-cookie'] ?? [] + const setCookies = response.headers.getSetCookie() const shopifyEssential = getCookie(setCookies, '_shopify_essential') /** @@ -175,7 +175,7 @@ async function enrichSessionWithStorefrontPassword( ) } - const setCookies = response.headers.raw()['set-cookie'] ?? [] + const setCookies = response.headers.getSetCookie() const storefrontDigest = getCookie(setCookies, 'storefront_digest') const newShopifyEssential = getCookie(setCookies, '_shopify_essential') diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 4fd77aa6ac6..e21ec1e9801 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -249,6 +249,9 @@ importers: '@vitest/coverage-istanbul': specifier: ^3.2.7 version: 3.2.7(vitest@4.1.10) + undici: + specifier: 8.10.0 + version: 8.10.0 packages/cli: dependencies: @@ -388,9 +391,6 @@ importers: find-up: specifier: 6.3.0 version: 6.3.0 - form-data: - specifier: 4.0.6 - version: 4.0.6 fs-extra: specifier: 11.1.0 version: 11.1.0 @@ -439,9 +439,6 @@ importers: network-interfaces: specifier: 1.1.0 version: 1.1.0 - node-fetch: - specifier: 3.3.2 - version: 3.3.2 open: specifier: 8.4.2 version: 8.4.2 @@ -466,6 +463,9 @@ importers: supports-hyperlinks: specifier: 3.2.0 version: 3.2.0 + undici: + specifier: 8.10.0 + version: 8.10.0 which: specifier: 4.0.0 version: 4.0.0 @@ -642,6 +642,9 @@ importers: '@vitest/coverage-istanbul': specifier: ^3.2.7 version: 3.2.7(vitest@4.1.10) + undici: + specifier: 8.10.0 + version: 8.10.0 packages/plugin-did-you-mean: dependencies: @@ -8554,6 +8557,10 @@ packages: resolution: {integrity: sha512-cRZYrTDwWznlnRiPjggAGxZXanty6M8RV1ff8Wm4LWXBp7/IG8v5DnOm74DtUBp9OONpK75YlPnIjQqX0dBDtA==, tarball: https://registry.npmjs.org/undici/-/undici-7.28.0.tgz} engines: {node: '>=20.18.1'} + undici@8.10.0: + resolution: {integrity: sha512-HvltHd7avK13QIw/oLe4qoOLyoVSoafqJ2jYOrtMRBkbYT31eiBQ8O0ehRKZiEZCMEyLFQNIADpgCWC5fALvYQ==, tarball: https://registry.npmjs.org/undici/-/undici-8.10.0.tgz} + engines: {node: '>=22.19.0'} + unicode-canonical-property-names-ecmascript@2.0.1: resolution: {integrity: sha512-dA8WbNeb2a6oQzAQ55YlT5vQAWGV9WXOsi3SskE3bcCdM0P4SDd+24zS/OCacdRq5BkdsRj9q3Pg6YyQoxIGqg==, tarball: https://registry.npmjs.org/unicode-canonical-property-names-ecmascript/-/unicode-canonical-property-names-ecmascript-2.0.1.tgz} engines: {node: '>=4'} @@ -18016,6 +18023,8 @@ snapshots: undici@7.28.0: optional: true + undici@8.10.0: {} + unicode-canonical-property-names-ecmascript@2.0.1: {} unicode-match-property-ecmascript@2.0.0: