Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove cross-fetch dependency #1658

Closed
flevi29 opened this issue May 23, 2024 · 0 comments · Fixed by #1688
Closed

Remove cross-fetch dependency #1658

flevi29 opened this issue May 23, 2024 · 0 comments · Fixed by #1688
Labels
enhancement New feature or request

Comments

@flevi29
Copy link
Collaborator

flevi29 commented May 23, 2024

As detailed in #1610, cross-fetch isn't helping much. The only thing it does is use node-fetch on older Node.js versions.

Also we are using two additional web standards that won't run on IE11 and we're not polyfilling:

const constructURL = new URL(url, this.url)
if (params) {
const queryParams = new URLSearchParams()
Object.keys(params)
.filter((x: string) => params[x] !== null)
.map((x: string) => queryParams.set(x, params[x]))
constructURL.search = queryParams.toString()
}

I propose we remove this polyfill, users of this library can import a polyfill script for fetch (browser) (node) and the other stuff themselves in the few cases where they might still need to do so, and if they're targeting IE11, they already need to do this with URL https://github.com/lifaon74/url-polyfill. I believe polyfilling should be the responsibility of the final apps/websites.

#1656 is kind of a blocker for this, as removing cross-fetch would break tests, and that PR already fixes them.

@flevi29 flevi29 added the enhancement New feature or request label May 23, 2024
meili-bors bot added a commit that referenced this issue Sep 15, 2024
1688: Remove cross-fetch dependency r=brunoocasali a=flevi29

# Pull Request

## Related issue
Fixes #1658 

## What does this PR do?
- removes `cross-fetch` dependency
- if anyone might need the polyfill they should run it themselves
- there's an interesting discussion [here](w3ctag/polyfills#6 (comment)) about polyfills in libraries; I think, as a lot of people do, that the sane and clean path ahead is to make it the responsibility of the library user to polyfill, even if it's somewhat of a burden

## PR checklist
Please check if your PR fulfills the following requirements:
- [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
- [x] Have you read the contributing guidelines?
- [x] Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!


1698: Add `"packageManager"` entry to package.json r=brunoocasali a=flevi29

# Pull Request

## Why?
- [`corepack`](https://nodejs.org/docs/latest-v20.x/api/corepack.html) [started adding this field automatically](nodejs/corepack#413), which can be disabled, but everyone has to do it separately, which is very annoying
- anyhow there's no harm in it, `corepack` recommends it even in the "how to disable" doc https://github.com/nodejs/corepack/blob/main/README.md#environment-variables
  - > it ensures that your project installs are always deterministic (if you use `corepack`)
- this does mean that this field will have to be updated periodically, but this is the recommended way

## PR checklist
Please check if your PR fulfills the following requirements:
- [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
- [x] Have you read the contributing guidelines?
- [x] Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!


Co-authored-by: F. Levi <55688616+flevi29@users.noreply.github.com>
@meili-bors meili-bors bot closed this as completed in 9c8e0fc Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant