Cache breakdown responses in details modals#6274
Cache breakdown responses in details modals#6274RobertJoonas wants to merge 6 commits intomasterfrom
Conversation
|
|
|
||
| describe(`${getStaleTime.name}`, () => { | ||
| describe('realtime periods', () => { | ||
| it('for realtime', () => { |
There was a problem hiding this comment.
meganitpick, non-blocking: Can we refactor it( to test(? it works better if there's a verb right after
| getStaleTime | ||
| } from './api-client' | ||
|
|
||
| const site = DEFAULT_SITE |
There was a problem hiding this comment.
suggestion, non-blocking: If we refactor getStaleTime not to depend on arg named site but directly on the two properties from the site, it makes the test depend on less things. That way, we'd not need to import or fake a full site here, just the properties offset and statsBegin.
| const availableIntervals = validIntervals( | ||
| Object.fromEntries( | ||
| Object.entries(props).filter(([k, _v]) => k !== 'date') | ||
| ) as GetIntervalProps | ||
| ) |
There was a problem hiding this comment.
issue, blocking: The cleaner way to exclude date would be to destructure const {date, ...rest} = props, const availableIntervals = validIntervals(rest), but we don't actually have to do it, because the function ignores the date prop if present. It doesn't make use of it. Checking locally, the line const availableIntervals = validIntervals(props) is valid according to typescript compiler.
|
|
||
| const { selectedInterval, onIntervalClick, availableIntervals } = | ||
| useStoredInterval({ | ||
| useStoredInterval(intervalStorageKey, { |
There was a problem hiding this comment.
question, non-blocking: Why can't the hook contain the logic to derive the storage key? Seems totally within its limited responsibilities.
2 participants
Changes
now), give a much shorter stale time of 5min if 'day' (or shorter) graph interval is supported, or 15min otherwise. Use thevalidIntervalslogic for determining that, which also takes comparison period into account.usePaginatedGetAPI, enabling cache for all breakdown modals, including Google Search Terms. Note that the current prod behaviour serves results from cache as well, but due tostaleTime: 0those results are instantly considered stale and a background re-fetch is triggered. Therefore, in terms of UX, nothing changes, other than not flashing a loading spinner on cache-hit.Tests
Changelog
Documentation
Dark mode