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: add undici to satisfy peer dep requirements #78

Merged
merged 1 commit into from
Nov 23, 2022

Conversation

bajtos
Copy link
Contributor

@bajtos bajtos commented Nov 23, 2022

The module native-fetch requires undici to be installed as its peer dependency.

Before this change, pnpm add was reporting a missing peer dependency.

dependencies:
+ dns-over-http-resolver 2.1.0

 WARN  Issues with peer dependencies found
.
└─┬ dns-over-http-resolver 2.1.0
  └─┬ native-fetch 4.0.2
    └── ✕ missing peer undici@"*"
Peer dependencies that should be installed:
  undici@"*"

With this change in place, there are no warnings reported.

dependencies:
+ dns-over-http-resolver 2.1.1

Progress: resolved 9, reused 9, downloaded 0, added 9, done

To be honest, I think it would be better to add undici as a dependency to native-fetch, as that would fix the problem for all consumers of native-fetch. However, in libp2p/js-libp2p#1487, @achingbrain suggested to fix the problem in this repo, so here we are.

Fixes #44

The module `native-fetch` requires `undici` to be installed as its peer
dependency.

Before this change, `pnpm add` was reporting a missing peer dependency.

    dependencies:
    + dns-over-http-resolver 2.1.0

     WARN  Issues with peer dependencies found
    .
    └─┬ dns-over-http-resolver 2.1.0
      └─┬ native-fetch 4.0.2
        └── ✕ missing peer undici@"*"
    Peer dependencies that should be installed:
      undici@"*"

With this change in place, there are no warnings reported.

    dependencies:
    + dns-over-http-resolver 2.1.1

    Progress: resolved 9, reused 9, downloaded 0, added 9, done

Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
@achingbrain
Copy link
Collaborator

To be honest, I think it would be better to add undici as a dependency to native-fetch

This was done on purpose to allow consumers to control the version of undici they use. It's more of a legacy thing from when native-fetch used node-fetch which sometimes had interesting bugs in certain versions.

node 18 has global fetch so hopefully this whole sorry mess will go away soon, likely when electron ships with node 18 (landed in v23 nightlies a couple of weeks ago, so probably in a couple of months time?).

@achingbrain achingbrain merged commit cf7a941 into vasco-santos:main Nov 23, 2022
github-actions bot pushed a commit that referenced this pull request Nov 23, 2022
## [2.1.1](v2.1.0...v2.1.1) (2022-11-23)

### Bug Fixes

* add `undici` to satisfy peer dep requirements ([#78](#78)) ([cf7a941](cf7a941)), closes [#44](#44)
@github-actions
Copy link

🎉 This PR is included in version 2.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@bajtos bajtos deleted the fix-peer-deps-warning branch November 23, 2022 13:52
@bajtos
Copy link
Contributor Author

bajtos commented Nov 23, 2022

To be honest, I think it would be better to add undici as a dependency to native-fetch

This was done on purpose to allow consumers to control the version of undici they use. It's more of a legacy thing from when native-fetch used node-fetch which sometimes had interesting bugs in certain versions.

Thank you for explaining the context 👍🏻

node 18 has global fetch so hopefully this whole sorry mess will go away soon, likely when electron ships with node 18 (landed in v23 nightlies a couple of weeks ago, so probably in a couple of months time?).

I am looking forward to that!

At the moment, native-fetch does not detect native fetch implementation in Node.js 18. Would you like me to open a pull request for that?

A possible blocker is that native fetch is still considered as an experimental feature in Node.js 18 and prints an ugly notice to stdout when used:

$ node
Welcome to Node.js v18.12.1.
Type ".help" for more information.
> await fetch('http://libp2p.io/')
(node:68539) ExperimentalWarning: The Fetch API is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
Response {
[...]

Compare that with fetch from undici, which does not print any warnings:

$ node
Welcome to Node.js v18.12.1.
Type ".help" for more information.
> const {fetch} = await import('undici')
undefined
> await fetch('http://libp2p.io/')
Response {
[...]

achingbrain pushed a commit that referenced this pull request Nov 6, 2023
Node 18 now has [global fetch](https://nodejs.org/en/blog/announcements/v18-release-announce) and given that electron now ships with [node 18 by default](electron/electron#36924), we don't need the polyfill anymore.

Related: #78

BREAKING CHANGE: requires node 18+
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node-fetch missing in dependency tree
2 participants