After #4120 was merged, here's a bit of polish on top.
- docs: Optimize JSDoc and comments.
- test: The existing 304 test is moved to the dedicated status code
block and extended with an `errorSpy` to also verify that no `error`
event is emitted.
- refactor: The success branch is moved before the error branch in
`fetch()`. This is more intuitive.
The bottom line is that this PR improves maintainability. For users,
this PR doesn't change anything.
## Summary
This updates `js/http_fetcher.js` so HTTP `304 Not Modified` responses
are emitted through the normal `response` event instead of being treated
as errors.
That aligns the core fetcher with the built-in `yr` weather provider in
`v2.35.0`, which already includes explicit logic to handle `304` and
reuse cached data.
Closes#4119.
## Root cause
`HTTPFetcher` currently treats all non-OK responses as errors. Since
`304` is not an OK response, it never reaches providers listening on the
`response` event.
At the same time, `defaultmodules/weather/providers/yr.js` expects to
receive `304` and handle it by reusing cached data.
## What changed
- special-case HTTP `304` in `HTTPFetcher`
- emit `304` through the normal `response` event
- preserve the existing error path for other non-OK statuses
## Validation
- confirmed `defaultmodules/weather/providers/yr.js` already expects
`response.status === 304`
- compared the unpatched `v2.35.0` `HTTPFetcher` behavior to the
provider logic
- ran `node --check js/http_fetcher.js`
Co-authored-by: sonnyb9 <sonnyb9@users.noreply.github.com>
As reported in #4109, the weather module retries much more frequently
than expected after network errors. #4092 already fixed the main cause
(duplicate fetchers), but the backoff logic in `HTTPFetcher` still has a
gap: once retries are exhausted, `calculateBackoffDelay` keeps returning
a short fixed delay (60s) instead of falling back to `reloadInterval`.
The same problem existed for 5xx errors, where the delay grew to 8× the
configured interval.
Inspired by #4110 (thanks @CodeLine9), this PR makes both error paths
fall back to `reloadInterval` after retries are exhausted. I also
simplified the catch block, extracted a `#shortenUrl()` helper for log
messages, and added tests for the backoff progression.
### What's the problem?
The `selfSignedCert` option passes an undici `Agent` as `dispatcher` to
`fetch()`. But Node's built-in `fetch()` and undici@8's `Agent` use
different internal handler APIs - passing them together throws:
```
invalid onRequestStart method
```
### What's the fix?
When `selfSignedCert` is enabled (i.e. a `dispatcher` is set), use
undici's own `fetch()` instead of the global one. For all other
requests, keep using `globalThis.fetch`.
```js
const fetchFn = requestOptions.dispatcher ? undiciFetch : globalThis.fetch;
```
### Why not just always use undici's fetch?
That would fix the crash - but it would break some tests. MSW (Mock
Service Worker), which is used in our test suite to intercept HTTP
requests, only hooks into `globalThis.fetch`. Undici's fetch bypasses
those interceptors entirely, so tests would start making real network
requests instead of getting the mocked responses. We could rewrite all
tests to use undici-compatible mocking instead - but that would be a
massive change for no real benefit.
----
Fixes#4093
- remove param `--enable-features=UseOzonePlatform` in start electron
tests (as we did already in `package.json`)
- update node versions in github workflows, remove `22.21.1`, add `25.x`
- fix formatting in tests
- update dependencies including electron to v40
This is still a draft PR because most calendar electron tests are not
running which is caused by the electron update from `v39.3.0` to
`v40.0.0`. Maybe @KristjanESPERANTO has an idea ...
---------
Co-authored-by: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com>
## Summary
PR [#3976](https://github.com/MagicMirrorOrg/MagicMirror/pull/3976)
introduced smart HTTP error handling for the Calendar module. This PR
extracts that HTTP logic into a central `HTTPFetcher` class.
Calendar is the first module to use it. Follow-up PRs would migrate
Newsfeed and maybe even Weather.
**Before this change:**
- ❌ Each module had to implemented its own `fetch()` calls
- ❌ No centralized retry logic or backoff strategies
- ❌ No timeout handling for hanging requests
- ❌ Error detection relied on fragile string parsing
**What this PR adds:**
- ✅ Unified HTTPFetcher class with intelligent retry strategies
- ✅ Modern AbortController with configurable timeout (default 30s)
- ✅ Proper undici Agent for self-signed certificates
- ✅ Structured error objects with translation keys
- ✅ Calendar module migrated as first consumer
- ✅ Comprehensive unit tests with msw (Mock Service Worker)
## Architecture
**Before - Decentralized HTTP handling:**
```
Calendar Module Newsfeed Module Weather Module
┌─────────────┐ ┌─────────────┐ ┌─────────────┐
│ fetch() own │ │ fetch() own │ │ fetch() own │
│ retry logic │ │ basic error │ │ no retry │
│ error parse │ │ handling │ │ client-side │
└─────────────┘ └─────────────┘ └─────────────┘
│ │ │
└───────────────────────┴───────────────────────┘
▼
External APIs
```
**After - Centralized with HTTPFetcher:**
```
┌─────────────────────────────────────────────────────┐
│ HTTPFetcher │
│ • Unified retry strategies (401/403, 429, 5xx) │
│ • AbortController timeout (30s) │
│ • Structured errors with translation keys │
│ • undici Agent for self-signed certs │
└────────────┬──────────────┬──────────────┬──────────┘
│ │ │
┌───────▼───────┐ ┌────▼─────┐ ┌──────▼──────┐
│ Calendar │ │ Newsfeed │ │ Weather │
│ ✅ This PR │ │ future │ │ future │
└───────────────┘ └──────────┘ └─────────────┘
│ │ │
└──────────────┴──────────────┘
▼
External APIs
```
## Complexity Considerations
**Does HTTPFetcher add complexity?**
Even if it may look more complex, it actually **reduces overall
complexity**:
- **Calendar already has this logic** (PR #3976) - we're extracting, not
adding
- **Alternative is worse:** Each module implementing own logic = 3× the
code
- **Better testability:** 443 lines of tests once vs. duplicating tests
for each module
- **Standards-based:** Retry-After is RFC 7231, not custom logic
## Future Benefits
**Weather migration (future PR):**
Moving Weather from client-side to server-side will enable:
- **Same robust error handling** - Weather gets 429 rate-limiting, 5xx
backoff for free
- **Simpler architecture** - No proxy layer needed
Moving the weather modules from client-side to server-side will be a big
undertaking, but I think it's a good strategy. Even if we only move the
calendar and newsfeed to the new HTTP fetcher and leave the weather as
it is, this PR still makes sense, I think.
## Breaking Changes
**None**
----
I am eager to hear your opinion on this 🙂
