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

UMD bundled version theoretically should err on browsers without native fetch support #1610

Closed
flevi29 opened this issue Dec 17, 2023 · 3 comments · Fixed by #1688
Closed
Labels
maintenance Issue about maintenance (CI, tests, refacto...)

Comments

@flevi29
Copy link
Collaborator

flevi29 commented Dec 17, 2023

The UMD bundled version has the following code in it's http request class:

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

As far as I know browsers don't implement require, it's a Node.js specific module import function. It's supposed to run on IE11 and other decade old end of life browsers/runtimes, but it most probably won't.

Testing this with Firefox, Chrome where the check of if (typeof fetch === 'undefined') { is commented out, we get a predictable error:
Uncaught (in promise) ReferenceError: require is not defined
I am attaching this test file, where I have the UMD file copied into the script tag.
test.zip

While the situation can be mended, I still think it would be best and easiest to drop support for IE11 and target a sensible ES version, and simply drop cross-fetch (by the way it generates a warning in Angular meilisearch/meilisearch-js-plugins#1269), the oldest maintained Node.js version supports fetch out of the box without any warnings, fetch is widely supported at this point.

@flevi29
Copy link
Collaborator Author

flevi29 commented Dec 24, 2023

Upon further investigation, I found that cross-fetch is not UMD compatible. cross-fetch has the following in its package.json:

{
  // ...
  "main": "dist/node-ponyfill.js",
  "browser": "dist/browser-ponyfill.js",
  "react-native": "dist/react-native-ponyfill.js",
  "types": "index.d.ts",
  // ...
}

cross-fetch/polyfill:

{
  "name": "cross-fetch-polyfill",
  "version": "0.0.0",
  "main": "../dist/node-polyfill.js",
  "browser": "../dist/browser-polyfill.js",
  "react-native": "../dist/react-native-polyfill.js",
  "license": "MIT"
}

Our rollup.config.js:

// ...
      nodeResolve({
        mainFields: ['jsnext', 'main'],
        preferBuiltins: true,
        browser: true,
      })
// ...

https://github.com/rollup/plugins/tree/master/packages/node-resolve#browser

In a built UMD nothing should be left as an import or require (unless we're absolutely sure that these only get called in an environment that supports them), that's the whole point, needs to run everywhere, IE11 has no concept of any kind of module system. So the way it is currently won't work on older browsers, which is fine by me, but that pretty much defeats the whole point of cross-fetch, specifically its browser oriented code that uses XHR to mimic fetch.

On the other hand if we try to bundle the code into the UMD, it can only bundle one of the two exported polyfills, either the node one or the browser one. Currently because of nodeResolve browser: true, the browser export gets chosen. This means that it will not run on node, because node doesn't implement XHR.

We could still potentially make it work, but then we'd have to bundle whatwg-fetch (the one used by cross-fetch for browser environments), and have our own polyfill that detects the environment, and based on that use the aforementioned fetch or native fetch if there is one, or node-fetch if we're in a node environment.

I really want to recommend against all of this, the package is already over-complicated in my opinion, and the chances that someone uses this on IE11, or even older Node.js environments is growing slimmer and slimmer by the day, as all of that is EOL. If they want to use older Node.js versions they are welcome to stick to older versions of this package.

@flevi29
Copy link
Collaborator Author

flevi29 commented Dec 24, 2023

I find it funny that the last maintainer posted on stackoverflow about this problem: https://stackoverflow.com/questions/63594241/how-to-create-umd-bundle-with-fetch-polyfill-rollup-and-typescript . Failed to gauge the exact details of the issue.

@flevi29
Copy link
Collaborator Author

flevi29 commented Dec 24, 2023

I'm pretty sure that the last paragraph in the installation section refers to the whole project not being UMD compatible:
https://github.com/lquixada/cross-fetch#install

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Issue about maintenance (CI, tests, refacto...)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants