i18next / i18next-http-backend

i18next-http-backend is a backend layer for i18next using in Node.js, in the browser and for Deno.
MIT License
454 stars 70 forks source link

Remove [cross-fetch] dependency. #104

Closed sosoba closed 1 year ago

sosoba commented 1 year ago

🚀 Feature Proposal

Remove cross-fetch dependency.

Motivation

Nowadays Node 18 LTS has global fetch enabled by default. Deno has also always had global fetch.

The cross-fetch has a hard coded node-fetch subdepndency which are redundant luggage. If someone need to support old Node versions they can add ponyfil directly in our legacy project:

npm i node-fetch
if (!global.fetch) {
  global.fetch = require('node-fetch');
}
adrai commented 1 year ago

cross-fetch (node-fetch) is not used if global fetch is available... so there should be no problem with that?

sosoba commented 1 year ago

The problem is the hard dependence on node-fetch. Npm always install thist package even hardly anyone needs it anymore. This dependency should be optional.

adrai commented 1 year ago

And what's the problem if it's installed? It should not be bundled, right?

adrai commented 1 year ago

tldr;

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

jonkoops commented 1 year ago

@adrai now that the Fetch API has been marked as stable in Node.js 21, and this API is available unflagged in all actively supported LTS versions (v18, v20), would you reconsider removing this dependency?

adrai commented 1 year ago

I still don't see any disadvantage of keeping it. Only because node-fetch or cross-fetch is installed in your node_modules, does not mean it gets loaded by the runtime or the browser.

btw: v2.3.0 uses the new cross-fetch update: https://github.com/lquixada/cross-fetch/releases/tag/v4.0.0 that officially included node v18 and v20 support

jonkoops commented 1 year ago

I still don't see any disadvantage of keeping it. Only because node-fetch or cross-fetch is installed in your node_modules, does not mean it gets loaded by the runtime or the browser.

True, this would only remove the dependency from the package so that less code needs to be downloaded. I think that is still a valid thing to do, but perhaps should be part of a major version (if any will be made at some point).

btw: v2.3.0 uses the new cross-fetch update: https://github.com/lquixada/cross-fetch/releases/tag/v4.0.0 that officially included node v18 and v20 support

Interestingly it also drops support for Node.js 10 and 12 in that version. Are the supported Node.js versions for 18next-http-backend documented somewhere? I can see 16, 18 and 20 in the CI.

adrai commented 1 year ago

I can see 16, 18 and 20 in the CI.

Yes, those there are always tested, but also lower versions might work.

jonkoops commented 1 year ago

Just ran into this strange issue with cross-fetch and Vite due to it being a dependency of this package. It looks like with Vite 5 somehow both cross-fetch and node-fetch are included in production builds, which results in the following error:

Uncaught ReferenceError: global is not defined

It appears that cross-fetch depends on an older version of node-fetch, which attempts to access the global variable (that doesn't exist in browsers).

adrai commented 1 year ago

Can you please provide a reproducible example?

jonkoops commented 1 year ago

Sure, yeah working on that now.

jonkoops commented 1 year ago

Managed to reproduce this locally. Looks like this is due to some overrides we're setting in out Vite config, so I don't believe this is cause for concern.

flipvh commented 9 months ago

Hi @sosoba Sorry to bother you, but I came here looking to reduce my js bundle size a bit as I use this nice package in the browser. (Or should I not use this for frontend?) I noticed cross-fetch is quite a large dependency. Have you found a way to reduce bundle size yourself with this package or in another way? Thanks!!

adrai commented 9 months ago

ceoss-fetch is not loaded on runtime if native fetch implementation is available

jacobbogers commented 9 months ago

Hi, Can you expose the "fetch" as pluggable, this would help greatly at testing especially as we dont use jest mocks (it.concurrent will be an issue since jest mocks are global to test file) hence the need to make it pluggable

Example:

config: { 
    getFetch: () => globalThis.fetch
}

for mocking (integration tests)

config: { 
    getFetch: () => fetchMocked
}