Allow the SDK to use native fetch

[icyJoseph]

Description

As of Node 18.x.x it is possible that fetch is available in the global object, making the cross-fetch polyfill unnecessary.

Basic example

On these lines: https://github.com/meilisearch/meilisearch-js/blob/795044940b1c3c0a11ee62da87de377668b5f5ed/src/http-requests.ts#L141C1-L148C9

      const fetchFn = this.httpClient ? this.httpClient : fetch
      const result = fetchFn(constructURL.toString(), {
        ...config,
        ...this.requestConfig,
        method,
        body,
        headers,
      })

Since the above happens within an async context, we could perhaps do something like this pseudocode:

      if (typeof fetch === 'undefined') {
            await import('cross-fetch/polyfill')
      }
      const fetchFn = this.httpClient ? this.httpClient : fetch
      const result = fetchFn(constructURL.toString(), {
        ...config,
        ...this.requestConfig,
        method,
        body,
        headers,
      })

I am not sure why, but this file src/errors/meilisearch-communication-error.ts also has that polyfill.

The main motivation for this, would be that in turn cross-fetch uses node-fetch, which in turn loads other modules and such, and not all of them are necessarily at their latest versions. For example node-fetch has a v3 version out there, and since Node 18, this chain of dependencies is avoidable, by the presence of a native fetch.

I guess this constitutes a breaking change though...

[icyJoseph]

It looks like it works, running docker-compose run --rm package bash -c "yarn install && yarn test && yarn lint", with both node:16 and node:18 in the docker-compose.yml file.

Though the implementation looks like:

    if (typeof fetch === 'undefined') {
        // @ts-expect-error polyfill brings in no meaningful types
        await import('cross-fetch/polyfill')
      }

      const fetchFn = this.httpClient ? this.httpClient : fetch
     

[brunoocasali]

Hello @icyJoseph just to mention the maintainer of this repo is on holiday! 🌴🌴

When they are back they will check your issue and your PR! ;)

[bidoubiwa]

I'm re-opening this issue. The code has been rollbacked. It introduced a failing built in some environments. See here #1496.

I didn't have time to look into it more and considered a release without that critical bug should be done as fast as possible.

See release v0.32.5

[icyJoseph]

I haven't really worked with unenv, but if I understand correctly, rather than installing cross-fetch, it looks it up inside its presets, and there it only has node_modules/unenv/runtime/npm/cross-fetch.cjs is that's what's going on with #1496?

• https://github.com/unjs/unenv#npm-packages

I'll do a bit of local testing, even within a Nitro env, when I get some time, because I think require('cross-fetch/polyfill') would suffice - I'll review why I went with a different choice (cross-fetch/dist/node-polyfill), than what I had originally proposed (see my comments above).

Looking forward

For what is worth, this change did not work for my use case either 😅 - Using Next.js with Node 18 I keep getting warnings about encoding not being resolved:

./node_modules/meilisearch/node_modules/node-fetch/lib/index.js
Module not found: Can't resolve 'encoding' in '/path-to-my/user/project/node_modules/meilisearch/node_modules/node-fetch/lib'

Import trace for requested module:
./node_modules/meilisearch/node_modules/node-fetch/lib/index.js
./node_modules/meilisearch/node_modules/cross-fetch/dist/node-ponyfill.js
./node_modules/meilisearch/node_modules/cross-fetch/dist/node-polyfill.js
./node_modules/meilisearch/dist/bundles/meilisearch.cjs.js
./src/posts/lib.ts
./src/app/og-image/[slug]/route.tsx

It still compiles and works fine though, just like it did before.

What makes this a bit more annoying is that, supabase also has this issue. It is not surprising because, fetch polyfills have been widely used the past years, so migrating away from them won't be so trivial.

I wonder if, looking forward, dropping the fetch polyfill, ought to be part of 1.x.x kind of thing - Not really sure what the release plans, goals and milestones for this project are though.

[icyJoseph]

Alright I can confirm that doing:

    if (typeof fetch === 'undefined') {
      require('cross-fetch/polyfill')
    }

Fixes the issue. Tested locally with a Nitro app, w/ Node 16 and 18.

Even made sure to run a health check on the Meilisearch instance, from the Nitro app.

It also passes all tests.

Should I open a PR?

[bidoubiwa]

Hey @icyJoseph, definitely! Thanks a lot for the investigation

[CapitaineToinon]

Commenting here because I think it's related but using Meilisearch in a Next 13 app gives me this warning, despite cross-fetch not being imported:

./node_modules/.pnpm/[email protected]/node_modules/node-fetch/lib/index.js
Module not found: Can't resolve 'encoding' in '/Users/antoine/Documents/www/speedsouls-bachelor/node_modules/.pnpm/[email protected]/node_modules/node-fetch/lib'

Import trace for requested module:
./node_modules/.pnpm/[email protected]/node_modules/node-fetch/lib/index.js
./node_modules/.pnpm/[email protected]/node_modules/cross-fetch/dist/node-ponyfill.js
./node_modules/.pnpm/[email protected]/node_modules/cross-fetch/dist/node-polyfill.js
./node_modules/.pnpm/[email protected]/node_modules/meilisearch/dist/bundles/meilisearch.cjs.js
./lib/meilisearch/search.ts
./components/search/command.tsx
./components/navbar.tsx

Apparently superbase has a similar problem. Just a warning so nothing that prevents me from deploying but maybe there is a way to fix this from the meilisearch side.

[icyJoseph]

@CapitaineToinon this was what I set out to fix, but supporting all environments, while also conditionally importing was a bit more difficult than I initially thought.

See my comment above for more detail.

[CapitaineToinon]

Oh sorry I missed that message. Thank you for clearing that up! 👍

[icyJoseph]

No worries. There are now tests for various environments, including nitro and I understand the codebase better, so I might try it once again.