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

fix: use temporary node-fetch fork #3

Conversation

oliveriosousa
Copy link

@oliveriosousa oliveriosousa commented Jul 27, 2021

  • Change node-fetch dependency to the fork with fix´s

This change is needed to run the examples in the web sandboxes (like codesandbox and stackblitz) because ipfs depends on js-ipfs-utils and this one depends on node-fetch.

When testing the examples using the stackblitz it throws an error:

Unable to resolve package (node-fetch npm:@achingbrain/node-fetch@^2.6.4) Please try installing a different version or range.

This resolution is being address in the js-ipfs-utils (see: ipfs/js-ipfs-utils#136)

CC: @achingbrain @hugomrdias

Copy link

@hugomrdias hugomrdias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@achingbrain could you please have a look ?

@achingbrain
Copy link
Owner

achingbrain commented Aug 16, 2021

I think this module should depend on node-fetch and not an unmaintained fork of that module.

Unable to resolve package (node-fetch npm:@achingbrain/node-fetch@^2.6.4) Please try installing a different version or range.

The latest version of the fork is 2.6.7 - have you tried using that?

Alternatively, it looks like the problem you are trying to solve is that stackblitz/codesandbox don't support the "npm:module@version" syntax for dependency versions, you could link to the tarball directly? E.g:

  "node-fetch": "https://registry.npmjs.org/@achingbrain/node-fetch/-/node-fetch-2.6.7.tgz"

TBH I don't know if stackblitz/codesandbox support that format either but this repo feels like the wrong place to fix this problem.

Longer term we'll probably swap out node-fetch for the undici fetch command as it seems a little more 'official' and presents the same API (e.g. WHATWG streams), assuming it doesn't have any other problems.

It requires node 16 though, so it'll have to wait until node 16 becomes LTS, which is six weeks or so away.

@oliveriosousa oliveriosousa force-pushed the fix/temp-node-fetch-fork branch from 6ed1d88 to 7f529a6 Compare August 16, 2021 14:42
@oliveriosousa
Copy link
Author

oliveriosousa commented Aug 16, 2021

Hi @achingbrain @hugomrdias,

Thanks for the suggestion, I've already updated the package.json targeting the tgz as suggested.

I ran the commands:

  • npm run prepare : no issues
  • npm run test: all green 👍

This PR is optional, just adds types to the project.

Changes made on js-ipfs-util: https://github.com/ipfs/js-ipfs-utils/pull/136/files

@oliveriosousa oliveriosousa force-pushed the fix/temp-node-fetch-fork branch from 7f529a6 to ae813ec Compare August 16, 2021 14:52
src/index.d.ts Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@oliveriosousa oliveriosousa force-pushed the fix/temp-node-fetch-fork branch from ae813ec to f294ae4 Compare August 17, 2021 20:34
@achingbrain
Copy link
Owner

This module now uses undici and not node-fetch as its fetch fallback as it supports WHATGW streams.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants