[Bugfix] Implement next.js fetch compatibility #126

jhamon · 2023-09-26T19:21:07Z

Problem

Next.js seems to stub out the cross-fetch polyfill which has the effect of silently breaking code which depends on cross-fetch to make API requests.

Related bugs:

[Bug] v1 Index<T>.query throwing TypeError: Cannot read properties of undefined (reading 'text') during migration #124
v1 migration problems in Upgrade to @pinecone-database/pinecone version 1.1.0 pinecone-vercel-starter#14

Solution

I have added a getFetch utility which looks for a fetch implementation already loaded in the global scope and returns the cross-fetch implementation only as a last resort.

Now when using our client library in a next.js project, this should result in the @vercel/fetch implementation being used in favor of the disabled polyfill.
When using this client library in other environments where next.js is not being used, it still activates the cross-fetch polyfill.
Also, I've exposed an experimental configuration option for anyone who wants to pass in a different fetch implementation to use. This could open up some flexibility for people with specific network configurations who need the ability to configure the http client being used in some way. If the user provides a fetch implementation through the fetchApi config option, it will take priority over a fetch in the global scope or the cross-fetch polyfill.

Other changes

Added additional integration tests for query method
Added an explicit dependency on the encoding module to resolve a warning message similar to this one

Type of Change

Bug fix (non-breaking change which fixes an issue)

Test Plan

I am currently testing this change in the context of work-in-progress to migrate our vercel-starter example to using the v1 version of the client. pinecone-io/pinecone-vercel-starter#14

Checkout the PR branch from pinecone-vercel-starter in a sibling directory
Install my local changes to this client with npm install ../pinecone-ts-client
Try to run the starter project with npm run dev
Attempt to use the sample app and observe it progresses beyond the error related broken cross-fetch

jhamon · 2023-09-27T12:44:01Z

jest.config.js

@@ -10,7 +10,7 @@ module.exports = {
  testPathIgnorePatterns: ['src/integration'],
  testTimeout: 100000,
  verbose: true,
-  detectOpenHandles: true,
+  detectOpenHandles: false,


I started getting an error related to TLSWRAP after adding some more integration tests. I should follow up on this later, but for now I don't want to block on it.

According to docs it's a large performance hit and should be generally disabled anyways outside of debugging. Seems fine to me. 👍

zackproser · 2023-09-27T14:44:26Z

src/utils/fetch.ts

+    // other implementations are stubbed out.
+    return global.fetch;
+  } else {
+    // Use ponyfill as last resort


Suggested change

// Use ponyfill as last resort

// Use polyfill as last resort

This is an intentional spelling, actually. cross-fetch can be imported globally ("polyfill") or locally ("ponyfill") according to their readme. Seems smart not to make global changes in a library that could interfere with other stuff in peoples environments.

zackproser · 2023-09-27T14:47:09Z

src/utils/fetch.ts

+    return global.fetch;
+  } else {
+    // Use ponyfill as last resort
+    return crossFetch;


So just to confirm my understanding:

If the user provides fetch, use that

In Vercel, the global.fetch if statement will be true, so we're good in Vercel land (or do they actually provide config.fetchApi?)

When running locally in a Pinecone example app, cross-fetch will be used, so that network requests the client needs to initialize will succeed

Do I have that right?

That matches my understanding as well, although I don't think Vercel will be providing anything via config.fetchApi. Unless of course someone using Vercel opts to provide their own fetch.

My understanding is that next.js loads @vercel/fetch and it will be available as a global without the need to specifically import it. So the next.js scenario would be the second logical branch here. My testing with the vercel starter showed that this now seems to work as expected.

The ability to pass it through config is just an escape hatch for anyone that has weird/specific needs around the http client configuration that I don't have bandwidth to address at the moment.

Roger, thanks!

austin-denoble

Nice fix, Jen. Overall the code LGTM, I did run into issues while trying to test via the Vercel starter directly though:

This may be an issue on my end, still playing with it a bit. Let me know if there's anything obvious.

austin-denoble · 2023-09-27T14:52:14Z

jest.config.js

@@ -10,7 +10,7 @@ module.exports = {
  testPathIgnorePatterns: ['src/integration'],
  testTimeout: 100000,
  verbose: true,
-  detectOpenHandles: true,
+  detectOpenHandles: false,


According to docs it's a large performance hit and should be generally disabled anyways outside of debugging. Seems fine to me. 👍

austin-denoble · 2023-09-27T14:53:47Z

src/utils/fetch.ts

+    return global.fetch;
+  } else {
+    // Use ponyfill as last resort
+    return crossFetch;


That matches my understanding as well, although I don't think Vercel will be providing anything via config.fetchApi. Unless of course someone using Vercel opts to provide their own fetch.

austin-denoble · 2023-09-27T14:56:45Z

src/integration/data/deleteMany.test.ts

@@ -12,7 +8,12 @@ describe('deleteMany', () => {
  beforeAll(async () => {
    pinecone = new Pinecone();

-    await createIndexIfDoesNotExist(pinecone, INDEX_NAME);


This was pulled out since we can use the new suppressConflicts and just call the create regardless, right?

Yep, sorry I didn't explain that better.

austin-denoble · 2023-09-27T14:58:41Z

src/data/types.ts

+    // fetchApi is a complex type that I don't really want to recreate in the
+    // form of a json schema (seems difficult and error prone). So we will
+    // rely on TypeScript to guide people in the right direction here.
+    // But declaring it here as Type.Any() is needed to avoid getting caught
+    // in the additionalProperties check.
+    fetchApi: Type.Optional(Type.Any()),


Makes sense, thanks for the comment.

austin-denoble · 2023-09-27T15:02:00Z

src/utils/fetch.ts

+import crossFetch from 'cross-fetch';
+import type { PineconeConfiguration } from '../data';
+
+export const getFetch = (config: PineconeConfiguration) => {


question: Is it worth adding a tiny unit test file for this utility?

This is simple enough that I don't think unit-testing would tell us much. I think what would be more valuable here would be to have some next.js-specific integration testing. But I think it's too much additional scope to add as part of this PR. Will have to look into it as a follow-up item.

jhamon added 3 commits September 26, 2023 15:43

Add getFetch util for next.js compatibility

b327316

Add missing fetch.ts file

e736a43

Add more integration tests for query

ba9deb7

jhamon force-pushed the jhamon/nextjs-fetch-fix branch from a1090d0 to ba9deb7 Compare September 27, 2023 11:25

Add encoding dependency to suppress nextjs missing module error

53641bd

jhamon changed the title ~~[Bugfix] Add getFetch utility for next.js compatibility~~ [Bugfix] Implement next.js fetch compatibility Sep 27, 2023

jhamon commented Sep 27, 2023

View reviewed changes

jhamon added 2 commits September 27, 2023 08:48

Adjust test

4364371

Cleanup

3815990

jhamon marked this pull request as ready for review September 27, 2023 13:05

jhamon requested review from austin-denoble and zackproser September 27, 2023 13:05

zackproser reviewed Sep 27, 2023

View reviewed changes

austin-denoble approved these changes Sep 27, 2023

View reviewed changes

jhamon merged commit b34f41b into main Sep 27, 2023
18 checks passed

jhamon deleted the jhamon/nextjs-fetch-fix branch September 27, 2023 15:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bugfix] Implement next.js fetch compatibility #126

[Bugfix] Implement next.js fetch compatibility #126

jhamon commented Sep 26, 2023 •

edited

Loading

jhamon Sep 27, 2023

austin-denoble Sep 27, 2023

zackproser Sep 27, 2023

jhamon Sep 27, 2023

zackproser Sep 27, 2023

austin-denoble Sep 27, 2023

jhamon Sep 27, 2023 •

edited

Loading

zackproser Sep 28, 2023

austin-denoble left a comment

austin-denoble Sep 27, 2023

austin-denoble Sep 27, 2023

austin-denoble Sep 27, 2023

jhamon Sep 27, 2023

austin-denoble Sep 27, 2023

austin-denoble Sep 27, 2023

jhamon Sep 27, 2023

	// Use ponyfill as last resort
	// Use polyfill as last resort

[Bugfix] Implement next.js fetch compatibility #126

[Bugfix] Implement next.js fetch compatibility #126

Conversation

jhamon commented Sep 26, 2023 • edited Loading

Problem

Solution

Other changes

Type of Change

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhamon Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austin-denoble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhamon commented Sep 26, 2023 •

edited

Loading

jhamon Sep 27, 2023 •

edited

Loading