[feat] data layer#16801
Conversation
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
pettinarip
left a comment
There was a problem hiding this comment.
@corwintines this is looking really good.
I have one last architectural concern about how consumers will interact with the data-layer.
Right now, to get data you'd need to do this:
import { getData } from "@/data-layer/storage/getter"
import { FETCH_ETH_PRICE_TASK_ID } from "@/data-layer/api/fetchEthPrice"This leaks internal details (storage paths, task IDs) into app code and loses type safety.
Suggested approach
- Data-layer exports a clean public API (
src/data-layer/index.ts)
export async function getEthPrice(): Promise<EthPriceData | null>
export async function getL2beatData(): Promise<L2beatData | null>
// ...moreNo Next.js imports here — keeps it framework agnostic so we can extract it later if needed.
- Next.js adapter handles caching (
src/lib/data/index.ts)
import { unstable_cache } from "next/cache"
import * as dataLayer from "@/data-layer"
export const getEthPrice = unstable_cache(
() => dataLayer.getEthPrice(),
["eth-price"],
{ revalidate: 3600 }
)- App code uses the adapter
import { getEthPrice } from "@/lib/data"
const price = await getEthPrice() // cached + typed ✨Why
- Types flow automatically (no generics)
- Data-layer stays isolated, no framework deps
- Caching is the app's concern, not the data-layer's
- Easy to extract to its own service later
Thoughts?
| ## Files | ||
|
|
||
| - `index.ts` - TypeScript index file with task ID exports | ||
| - `*.json` - Individual mock data files for each task | ||
|
|
||
| ## Last Generated | ||
|
|
||
| 2025-12-05T21:54:56.350Z | ||
|
|
||
| ## Statistics | ||
|
|
||
| - Total tasks: 21 | ||
| - Successfully generated: 20 | ||
| - Not found: 1 | ||
|
|
There was a problem hiding this comment.
this doesn't look relevant to consumers
There was a problem hiding this comment.
Could be useful depending on if we need to keep track of the last time a mocks were created, but will remove for now.
| let store: ReturnType<typeof getStore> | null = null | ||
|
|
||
| function getBlobStore() { | ||
| if (!store) { |
There was a problem hiding this comment.
can use early returns to make this more readable
if (store) {
return store
}
// ... rest| } else { | ||
| // Try automatic configuration (works in Netlify environment) | ||
| // This will use NETLIFY_BLOBS_CONTEXT if available | ||
| store = getStore("data-layer") |
There was a problem hiding this comment.
Not sure if we are ever going to use this use case. Is this something you have in mind for other uses?
IMO we should always have a these vars. If not, throw to let us know something is not working.
There was a problem hiding this comment.
For consistency and clarity, following the mockStorage name, we should call this netlifyBlobsStorage.
| export const netlifyBlobsStorage: StorageImplementation & | ||
| GetStorageImplementation = { |
There was a problem hiding this comment.
current typing around the storage is a bit fragmented and confusing.
would suggest unifying these types in types.ts and have 1 interface for the Storage.
// types.ts file
import type { tasks } from "./registry"
export type TaskId = (typeof tasks)[number]["id"]
/** Metadata stored alongside data */
export interface StorageMetadata {
storedAt: string
}
/** Result shape when retrieving data with metadata */
export interface StorageResult<T> {
data: T
metadata: StorageMetadata
}
/**
* Unified storage interface.
* Implementations must provide both get and set methods.
*/
export interface Storage {
get<T>(taskId: TaskId): Promise<StorageResult<T> | null>
set(taskId: TaskId, data: unknown, metadata?: StorageMetadata): Promise<void>
}then the implementation would be cleaner
export const netlifyBlobsStorage: Storage = {
async get<T>(taskId: TaskId) { ... },
async set(taskId: TaskId, data: unknown, metadata?: StorageMetadata) { ... },
}| for (const task of dailyTasks) { | ||
| try { | ||
| console.log(`Fetching data for task: ${task.id}`) | ||
| const data = await task.fetchFunction() |
There was a problem hiding this comment.
we are running these sequentially which can be a major bottleneck. We can parallelized by using await Promise.allSettled
pettinarip
left a comment
There was a problem hiding this comment.
@corwintines overall looks fine. I've noticed the addition of unit tests, left some questions
| } | ||
|
|
||
| // Set USE_MOCK_DATA before importing data-layer | ||
| process.env.USE_MOCK_DATA = "true" |
There was a problem hiding this comment.
lets avoid setting env vars in code. You are already doing "test:unit": "USE_MOCK_DATA=true playwright test --project=unit" in the scripts (which is the correct approach).
|
|
||
| // Register path aliases | ||
| try { | ||
| require("tsconfig-paths/register") |
There was a problem hiding this comment.
why do we need to register this? is this necessary? the e2e tests did not require it
| testDir: "./tests/e2e", | ||
| outputDir: "./tests/e2e/__results__", | ||
| testDir: "./tests", | ||
| outputDir: "./tests/__results__", |
There was a problem hiding this comment.
this is probably need that we update GH action yaml file as well, or not?
| if (result !== null) { | ||
| // MetricReturnData is ValueOrError<number> | ||
| expect(result).toHaveProperty("value") | ||
| expect(typeof result.value).toBe("number") |
There was a problem hiding this comment.
I get a bunch of TS errors coming mostly from accessing the value prop
tests/unit/data-layer/getters.spec.ts:23:30 - error TS2339: Property 'value' does not exist on type 'MetricReturnData'.
Property 'value' does not exist on type '{ error: string; }'.
pettinarip
left a comment
There was a problem hiding this comment.
@corwintines I think there’s one major blocker at the moment, related to the mocks and the amount of code being added in this PR (~54k new lines). We should probably sort that out before merging. Some of the mock files are over 500 lines, which feels a bit too big for what they need to do.
As for smaller things (that we can clean up later):
- some unnecessary boilerplate
- excessive comments
- verbose logging
All of that just makes the code a bit harder to follow. I’m sure we can simplify it quite a bit.
pettinarip
left a comment
There was a problem hiding this comment.
@corwintines I've trimmed a bit more the mocks. LGTM! gj 💪🏼

Description
Implements: