-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[core] refactor: extract and centralize HTTP fetcher #4016
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
[core] refactor: extract and centralize HTTP fetcher #4016
Conversation
- Extract HTTP logic from Calendar module into HTTPFetcher class - Migrate calendar module to use centralized HTTPFetcher - Add modern improvements: * AbortController with configurable timeout (default 30s) * undici Agent for self-signed certificates * Improved Retry-After header handling (respects server hints) - Implement intelligent retry strategies: * 401/403: 5x interval, min 30 minutes * 429: Retry-After header parsing, fallback 15 minutes * 5xx: Exponential backoff (max 3 retries) * 4xx: 2x interval, min 15 minutes - Provide structured error objects with translation keys - Comprehensive unit tests with msw (Mock Service Worker) - Tests cover all error scenarios and authentication methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome idea, I really like!
Just one thing: We dont add the default english translations to other languages. If a translation isnt available, it automatically takes the english one.
|
My idea was to make it easier for the translators, but you're right, that was not very clean. Instead of taking the easy way out and simply removing them, I used automatic translation (which has delivered very good results in the past) to translate the strings. In doing so, I even noticed duplicate strings in the pt translation, which I removed right away. |
|
This broke my calendar: [2026-01-22 23:27:21.769] [ERROR] [calendar] https://calendar.google.com/calendar/ical/xxx.calendar.google.com/public/basic.ics - iCal parsing failed: UNTIL rule part MUST have the same value type as DTSTART
[2026-01-22 23:27:21.770] [ERROR] [calendar] Calendar Error. Could not fetch calendar: https://calendar.google.com/calendar/ical/xxx.calendar.google.com/public/basic.ics iCal parsing failed: UNTIL rule part MUST have the same value type as DTSTART |
|
Found the breaking entry: DTSTART;VALUE=DATE:20160313
DTEND;VALUE=DATE:20160314
RRULE:FREQ=YEARLY;UNTIL=20190312;BYMONTHDAY=13;BYMONTH=3
DTSTAMP:20260122T223427Z
UID:a21aa3d4-c1c2-4d74-b929-535e168a566f
CREATED:20170127T230833Z
LAST-MODIFIED:20190330T190642Z
SEQUENCE:1
STATUS:CONFIRMED
SUMMARY:xxx
TRANSP:OPAQUE
X-MOZ-FAKED-MASTER:1
X-MOZ-GENERATION:2
END:VEVENTI was able to solve the problem by deleting and recreating the entire calendar series in Google Calendar, but other users might encounter similar issues... |
|
Oh. I'll take a look at it by the weekend at the latest. |
|
don't know if it is a real problem, that was the only entry with a |
|
I think it is confused between full day and not for some reason, rrule until doesn’t use DATE: as it’s not required there |
Summary
PR #3976 introduced smart HTTP error handling for the Calendar module. This PR extracts that HTTP logic into a central
HTTPFetcherclass.Calendar is the first module to use it. Follow-up PRs would migrate Newsfeed and maybe even Weather.
Before this change:
fetch()callsWhat this PR adds:
Architecture
Before - Decentralized HTTP handling:
After - Centralized with HTTPFetcher:
Complexity Considerations
Does HTTPFetcher add complexity?
Even if it may look more complex, it actually reduces overall complexity:
calendarfetcherutils.js#3976) - we're extracting, not addingFuture Benefits
Weather migration (future PR):
Moving Weather from client-side to server-side will enable:
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 🙂