Add conditional loading for cross-fetch/polyfill - favour native fetch

[icyJoseph]

Pull Request

Related issue

Fixes #1487

What does this PR do?

Load cross-fetch/polyfill only if fetch is undefined.

PR checklist

Please check if your PR fulfills the following requirements:

• Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)? Allow the SDK to use native fetch #1487 was opened by me though...
• Have you read the contributing guidelines?
• Have you made sure that the title is accurate and descriptive of the changes?

[bidoubiwa]

Hey @icyJoseph, you are right. Until v16 is end of life, we should keep the possibility to use the polyfill but since v18 is the latest node version, users shouldn't have to download the polyfill if they are in v18 and above.

I'm reviewing your PR soon :) I was on holiday. Thanks for your contribution btw 🙏

[bidoubiwa]

Hey @icyJoseph, I was trying things out. I just realised that there is one file that I created that is not being ran by the CI (nor is it present in the package.json).

Could you try the following:

yarn build
cd tests/env/node
node getting_started.js

It raises the following error:

(node:43834) UnhandledPromiseRejectionWarning: ReferenceError: Response is not defined

If it's oke for you, could you update this line in the package.json:

    "test:env:nodejs": "node tests/env/node/index.js",

with

    "test:env:nodejs": "node tests/env/node/index.js && node tests/env/node/getting_started.js",

[icyJoseph]

Right, so that's why the src/errors/meilisearch-communication-error.ts file imported the polyfill, I did not check if there was usage of Response, Request, or Headers, which are also typically polyfilled.

I'll see if there's a better place to conditionally load the polyfill.

[bidoubiwa]

Thanks 🙏

[icyJoseph]

I switch to requiring conditionally, and now I see this error:

class_1 [MeiliSearchApiError]: Attribute `id` is not filterable. Available filterable attributes are: `genres director`.
1:3 id >= 1

When running:

yarn build
cd tests/env/node
node getting_started.js

And then it fails with a 400 error

[bidoubiwa]

There is apparently an error in the getting_started.js file.

Could you change line 21 with:

  await index.updateFilterableAttributes([
    'director',
    'genres',
    'id'
  ])

I also realise that yarn build should be added in the package.json command to ensure it uses the latest umd build

"test:env:nodejs": "yarn build && node tests/env/node/index.js && node tests/env/node/getting_started.js",

[icyJoseph]

Alright, I think it is looking good so far 😄

[bidoubiwa]

LGTM ✨✨✨✨

Thanks a lot

[bidoubiwa]

bors merge

[meili-bors]

Build succeeded:

• integration-tests (Node.js 14)
• integration-tests (Node.js 16)
• integration-tests (Node.js 18)
• style-check
• types-check