Data fetch refactor #16631
Data fetch refactor #16631
Conversation
…I and type definitions
…ntegrating blog feeds into daily data retrieval
…ine data handling across app pages
…s to streamline codebase
…ng and processing for improved data handling
…ons for calendar events, attestant posts, and blog feeds
…cing utility functions and streamlining API integration
…ion utility and removing deprecated fetch function
…growThePie data extraction process
…nd enhance data extraction utilities
…ion utilities, replacing deprecated methods
…new extraction utilities and enhance error handling
…line codebase and eliminate unused code
…n utilities, replacing deprecated methods and enhancing data handling
…n utilities, enhancing data handling and integrating launch dates into the network page
…hancing layer-2 data handling and improving network page functionality
…lities, enhancing framework data integration and improving local environment page functionality
…ted methods with new extraction utilities, enhancing overall data handling and integration
…ock data files for improved data handling and integration
…port paths and enhancing error handling, improving overall data integration across various pages
corwintines
requested review from
minimalsm,
pettinarip and
wackerow
as code owners
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
bot
added
config ⚙️
…roducing a utility function for time intervals
…nhance code maintainability
…on for improved testing capabilities
pettinarip
left a comment
pettinarip
left a comment
There was a problem hiding this comment.
Looking good, @corwintines! I’ve left a few observations. I haven’t tested it yet, so this is more of a general code review.
Minor: it would be great to add some basic documentation explaining how to consume data using the data layer.
app/api/external-data/route.ts
Outdated
There was a problem hiding this comment.
Do we still need this handler?
There was a problem hiding this comment.
Removed, had a local commit I hadnt pushed up
| // Note: unstable_cache creates a new cached function each time, but Next.js manages | ||
| // the cache entries based on the keyParts, so this is efficient | ||
| try { | ||
| const cachedFetch = unstable_cache( |
There was a problem hiding this comment.
I'm a bit unsure about this implementation since getExternalData creates a new function on each call, it likely won't be cached. The unstable_cache should instead wrap the exported function.
That said, I don't think the unstable_cache belongs at this level. It should be applied within each storage client implementation (eg getRedisData), since each client might use a different caching approach. For example, redis requires unstable_cache but a service using the native fetch API wouldn't need it. By placing the cache here, we're effectively locking ourselves into a specific caching strategy.
There was a problem hiding this comment.
Thanks for feedback. Pushed up refactor here around this that I believe addresses your points. Good point on making it service level, my bad.
|
|
||
| // Build the data map | ||
| const dataMap = results.reduce((acc, { key, data }) => { | ||
| if (data !== null && data !== undefined) { |
There was a problem hiding this comment.
Lets make sure we always return one value per key, even if it’s null or undefined, since those might be meaningful to the consumer. Or is that impossible to happen with current logic?
Im in favor of delegating error handling or custom logic to consumers.
There was a problem hiding this comment.
For organization purposes, would recommend placing all storage vendor files in a separate folder like data/clients.
src/lib/api/fetchL2beat.ts
Outdated
|
|
||
| return { | ||
| value: data, | ||
| timestamp: Date.now(), |
There was a problem hiding this comment.
Could this be handled by the fetchExternalData function? I think we should separate this strict return type from the fetcher functions.
We could do this automatically inside fetchExternalData:
const result = await service.function()
return { key: service.key, data: { value: result, timestamp: Date.now() } }That way we simplify the usage of the data layer and we garantee that all the calls with have the same data structure.
There was a problem hiding this comment.
This feels like an extra layer or glue code just to retrieve external data, and I think it could be simplified.
const [gfissuesResponse] = await getExternalData(["gfissues"], every("day"))
const { data: gfissues, timestamp, etc } = gfissuesResponseThe data you receive here depends entirely on what you return from your fetcher function. I'd still recommend keeping this logic inside each user-defined fetcher instead. That way, interacting with the data layer becomes simpler and more straightforward.
| { | ||
| protocol: "https", | ||
| hostname: "avatars.githubusercontent.com", | ||
| hostname: "avatars*.githubusercontent.com", |
There was a problem hiding this comment.
Just to double check if this is a desired change. Asking because it looks a bit out of context.
There was a problem hiding this comment.
I was getting issues with these images coming back from avatars1, avatars2, avatars3, or avatars4.githubusercontent.com, so I had just put this in here for now. I could make a separate PR for this too, but was running into it while developing this.
|
|
||
| if (upstashUrl && upstashToken) { | ||
| try { | ||
| const { Redis } = await import("@upstash/redis") |
There was a problem hiding this comment.
why do we import this dep like this?
would prefer to do as the docs:
import { Redis } from "@upstash/redis"
const redis = new Redis({
url: <UPSTASH_REDIS_REST_URL>,
token: <UPSTASH_REDIS_REST_TOKEN>,
})|
|
||
| if (supabaseUrl && supabaseAnonKey) { | ||
| try { | ||
| const { createClient } = await import("@supabase/supabase-js") |
There was a problem hiding this comment.
Same here, imports at the top of the file.
…configuration and improving caching strategies for Redis and Supabase
…sted keys in the data map, improving clarity on key presence and handling of null/undefined values
…moving deprecated files to streamline data fetching utilities
…s instead of wrapped objects, enhancing clarity and consistency across the API
…consolidating response handling and removing deprecated extraction utilities, enhancing code clarity and maintainability
Create a data layer
Refactor codebase to use data layer