fix: remove node-fetch, use native fetch (Node 22)#261
Open
shanelord01 wants to merge 1 commit intomauriceboe:devfrom
Open
fix: remove node-fetch, use native fetch (Node 22)#261shanelord01 wants to merge 1 commit intomauriceboe:devfrom
shanelord01 wants to merge 1 commit intomauriceboe:devfrom
Conversation
- Remove node-fetch dependency and all imports across 7 source files
- Use Node 22 native global fetch
- Replace custom https.Agent IPv4 workaround with dns.setDefaultResultOrder('ipv4first')
- Add NODE_OPTIONS=--dns-result-order=ipv4first to docker-compose as defence in depth
- Supersedes PR mauriceboe#180
https://claude.ai/code/session_013bW22pmABuR45SDcmVt2oZ
mauriceboe
requested changes
Apr 3, 2026
Owner
mauriceboe
left a comment
There was a problem hiding this comment.
Hey, thanks for the clean PR – solid work removing node-fetch in favor of native fetch, and the compatibility audit in the description is appreciated.
I switched the base branch to dev (that's where we merge feature/fix branches), but there are merge conflicts. Could you rebase onto dev so we can get this merged?
One minor note: dns.setDefaultResultOrder('ipv4first') in weather.ts is a process-wide side effect – it affects all DNS lookups, not just weather requests. Might be worth either moving it to the server entrypoint or at least adding a comment that it's global. Not a blocker though.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remove
node-fetch, use Node 22 nativefetchProblem
node-fetch@2.7.0pulls inwhatwg-url@5.0.0→tr46@0.0.3, which imports the built-inpunycodemodule deprecated since Node.js 21+. This produces the following warning on every server start:Fix
Removed
node-fetchentirely and switched to Node 22's native globalfetch. This eliminates the entire dependency chain (node-fetch,whatwg-url,tr46,webidl-conversions).Changes
node-fetchfrompackage.jsonandpackage-lock.jsonimport fetch from 'node-fetch'in 6 files:places.ts,oidc.ts,auth.ts,maps.ts,weather.ts,notifications.tsrequire('node-fetch')incollab.tswith globalfetchweather.ts, addeddns.setDefaultResultOrder('ipv4first')to preserve IPv4-first behaviour for Docker bridge networks where IPv6 cannot route to the internet (replaces the approach in PR fix: weather API reliability - cache, request queue, and IPv4 agent #180 which used a customhttps.Agent({ family: 4 }))Compatibility audit
All 7 source files use only standard fetch features (
json(),text(),ok,status,signal,redirect,headers). No node-fetch-specific classes (Response,Request,Headers) or stream handling were in use.redirect: 'error'confirmed compatible with nativefetch.Supersedes #180
Using Claude Code - please ignore or use - it's up to you.