From 7a74efd5fd7c6db53c9f2ca4a46b98b503d27a78 Mon Sep 17 00:00:00 2001 From: Audrey Sage Lorberfeld Date: Tue, 20 Aug 2024 14:41:14 -0700 Subject: [PATCH] Remove unnecessary crossFetch fallback (#250) ## Problem We made the decision a while back to [remove support for Node versions <18](https://github.com/pinecone-io/pinecone-ts-client/pull/239). Previously, in order to account for Node versions <18, w resorted to using a `ponyfill` called `crossFetch` for the `fetch` implementation, when no user-supplied implementation was found. Now that we no longer support Node versions <18, there's no need for us to fall back to `crossFetch`. ## Solution Remove the `crossFetch` fallback. ## Type of Change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update - [ ] Infrastructure change (CI configs, etc) - [ ] Non-code change (docs, etc) - [ ] None of the above: (explain here) ## Test Plan CI passes. --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1208002049018704 --- .github/actions/setup/action.yml | 2 +- .github/workflows/testing.yml | 4 +-- README.md | 4 +-- src/utils/__tests__/fetch.test.ts | 49 +++++++++++++++++++++++++++++++ src/utils/fetch.ts | 10 +++++-- 5 files changed, 60 insertions(+), 9 deletions(-) create mode 100644 src/utils/__tests__/fetch.test.ts diff --git a/.github/actions/setup/action.yml b/.github/actions/setup/action.yml index ac07ffb6..9fa0783b 100644 --- a/.github/actions/setup/action.yml +++ b/.github/actions/setup/action.yml @@ -8,7 +8,7 @@ runs: using: 'composite' steps: - name: Setup Node - uses: actions/setup-node@v3 + uses: actions/setup-node@v4 with: node-version: ${{ inputs.node_version }} registry-url: 'https://registry.npmjs.org' diff --git a/.github/workflows/testing.yml b/.github/workflows/testing.yml index c41cec46..cbb38f1f 100644 --- a/.github/workflows/testing.yml +++ b/.github/workflows/testing.yml @@ -78,7 +78,7 @@ jobs: - name: Checkout uses: actions/checkout@v4 - name: Setup Node - uses: actions/setup-node@v3 + uses: actions/setup-node@v4 with: node-version: 18.x registry-url: 'https://registry.npmjs.org' @@ -118,7 +118,7 @@ jobs: - name: Checkout uses: actions/checkout@v4 - name: Setup Node - uses: actions/setup-node@v3 + uses: actions/setup-node@v4 with: node-version: 18.x registry-url: 'https://registry.npmjs.org' diff --git a/README.md b/README.md index 589e8613..0baf0b6e 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,7 @@ when configuring an index has changed to include `deletionProtection`. The `podT ## Prerequisites -The Pinecone TypeScript SDK is compatible with TypeScript 4.1 and greater. +The Pinecone TypeScript SDK is compatible with TypeScript >=4.1 and Node >=18.x. ## Installation @@ -897,8 +897,6 @@ you access to embedding models hosted on Pinecone's infrastructure. Read more at **Notes:** -- The Inference API only works with Node `>=18`. If you are using older versions of Node, you must pass in a custom `fetch` implementation into the Pinecone client. - Models currently supported: - [multilingual-e5-large](https://arxiv.org/pdf/2402.05672) diff --git a/src/utils/__tests__/fetch.test.ts b/src/utils/__tests__/fetch.test.ts new file mode 100644 index 00000000..d3996e1e --- /dev/null +++ b/src/utils/__tests__/fetch.test.ts @@ -0,0 +1,49 @@ +import { getFetch } from '../fetch'; +import { PineconeConfigurationError } from '../../errors'; +import type { PineconeConfiguration } from '../../data'; + +describe('getFetch', () => { + afterEach(() => { + console.log('resetting global.fetch'); + // Reset global.fetch after each test to avoid affecting other tests + delete (global as any).fetch; + }); + + test('should return the user-provided fetch implementation if provided', () => { + const customFetch = jest.fn(); + const config = { + apiKey: 'some-api-key', + fetchApi: customFetch, + } as PineconeConfiguration; + + const fetchFn = getFetch(config); + + expect(fetchFn).toBe(customFetch); + }); + + test('should return the global fetch implementation if user-provided fetch is not present', () => { + const globalFetch = jest.fn(); + (global as any).fetch = globalFetch; + + const config = { + apiKey: 'some-api-key', + fetchApi: undefined, + } as PineconeConfiguration; + + const fetchFn = getFetch(config); + + expect(fetchFn).toBe(globalFetch); + }); + + test('should throw a PineconeConfigurationError if no fetch implementation is found', () => { + const config = { + apiKey: 'some-api-key', + fetchApi: undefined, + } as PineconeConfiguration; + + expect(() => getFetch(config)).toThrow(PineconeConfigurationError); + expect(() => getFetch(config)).toThrow( + 'No global or user-provided fetch implementations found. Please supply a fetch implementation.' + ); + }); +}); diff --git a/src/utils/fetch.ts b/src/utils/fetch.ts index 880342e5..33e06a83 100644 --- a/src/utils/fetch.ts +++ b/src/utils/fetch.ts @@ -1,5 +1,5 @@ -import crossFetch from 'cross-fetch'; import type { PineconeConfiguration } from '../data'; +import { PineconeConfigurationError } from '../errors'; export const getFetch = (config: PineconeConfiguration) => { if (config.fetchApi) { @@ -10,9 +10,13 @@ export const getFetch = (config: PineconeConfiguration) => { // scope, use that. This should prevent confusing failures in // nextjs projects where @vercel/fetch is mandated and // other implementations are stubbed out. + console.log( + 'Failed to find any user-provided fetch implementation. Using global fetch implementation.' + ); return global.fetch; } else { - // Use ponyfill as last resort - return crossFetch; + throw new PineconeConfigurationError( + 'No global or user-provided fetch implementations found. Please supply a fetch implementation.' + ); } };