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

How to use node-fetch-retry with @adobe/helix-fetch #75

Closed
trieloff opened this issue Mar 7, 2022 · 7 comments
Closed

How to use node-fetch-retry with @adobe/helix-fetch #75

trieloff opened this issue Mar 7, 2022 · 7 comments
Labels
enhancement New feature or request

Comments

@trieloff
Copy link

trieloff commented Mar 7, 2022

I'm working on a project with @alexkli and we would like to use @stefan-guggisberg's https://github.com/adobe/helix-fetch but also use this project's retry functionality.

This project has node-fetch hardcoded as a dependency, but as the dependency footprint is small, it should theoretically be possible to use a different fetch implementation.

I've tried using npm 8.3's override functionality to make replace node-fetch transparently with @adobe/helix-fetch, but ran into subtle incompatibilities:

  • node-fetch exports fetch as a default export, in @adobe/helix-fetch it is { fetch }
  • node-fetch can be required, @adobe/helix-fetch needs to be imported

I could imagine a pull request that would

  1. turn node-fetch into a peer dependency
  2. support an options.fetch parameter that allows passing a fetch implementation
  3. uses node-fetch as the default for this parameter
  4. require()s node-fetch dynamically (so that it is optional, but that might make packaging a bit more difficult)
@trieloff trieloff added the enhancement New feature or request label Mar 7, 2022
@trieloff
Copy link
Author

trieloff commented Mar 8, 2022

Thinking this a bit further, it would be cool to have the ability to run the entire library in the browser, using window.fetch

@stefan-guggisberg
Copy link

stefan-guggisberg commented Mar 9, 2022

node-fetch can be required, @adobe/helix-fetch needs to be imported

@trieloff Why do you think that @adobe/helix-fetchneeds to be imported?. The following code works without any issues:

const { fetch, reset } = require('@adobe/helix-fetch');

fetch('http://example.com')
  .then(async (resp) => console.log(`${resp.status} - ${await resp.text()}`))
  .finally(reset);

BTW: as of v3 node-fetch needs to be imported.

@stefan-guggisberg
Copy link

If the node-fetch dependency is going to be optional it would IMO make sense to make abort-controller optional as well.

@tmathern
Copy link
Member

Is browser support "just cool" or needed for your project?

@tmathern
Copy link
Member

tmathern commented Apr 25, 2022

Opened issue: ASSETS-9903

@alexkli
Copy link
Contributor

alexkli commented Apr 25, 2022

Is browser support "just cool" or needed for your project?

Not needed for helix-fetch in particular.

But it might be useful to have this project support browsers as well, if possible (might need to be renamed to fetch-retry then).

@alexkli
Copy link
Contributor

alexkli commented Apr 28, 2022

Closing in favor of #86.

@alexkli alexkli closed this as completed Apr 28, 2022
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

No branches or pull requests

4 participants