fix(ensapi): use native promises with swr cache#1301
Conversation
The SWR cache requires `fn` to return a native Promise object, so we cannot wrap the returned value with `pReflect`. On the other hand, in order to maintain the downstream logic which relies on `pReflect` result type, we still use `pReflect`, but this time on the middleware level, and not on the cache query fn level.
🦋 Changeset detectedLatest commit: 451b076 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…. Make it accept optional `onResolved` and `onRejected` callaback functions.
…`responseCode: IndexingStatusResponseCodes.Ok`.
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Thanks for your updates here. Reviewed and shared feedback 👍
| onResolved?: (value: T) => void; | ||
|
|
||
| /** | ||
| * Optional callback invoked when the wrapped function throws an error |
There was a problem hiding this comment.
| * Optional callback invoked when the wrapped function throws an error | |
| * Optional callback invoked when the wrapped function throws an error represented by `reason`. |
| * | ||
| * @param fn The async function to wrap with SWR caching | ||
| * @param ttl Time-to-live duration in seconds. After this duration, data is considered stale | ||
| * @param onResolved Optional callback invoked when the wrapped function resolves successfully |
There was a problem hiding this comment.
| * @param onResolved Optional callback invoked when the wrapped function resolves successfully | |
| * @param onResolved Optional callback invoked when `fn` resolves successfully (doesn't throw an error) |
Is that fair?
| * @param fn The async function to wrap with SWR caching | ||
| * @param ttl Time-to-live duration in seconds. After this duration, data is considered stale | ||
| * @param onResolved Optional callback invoked when the wrapped function resolves successfully | ||
| * @param onRejected Optional callback invoked when the wrapped function throws an error |
There was a problem hiding this comment.
| * @param onRejected Optional callback invoked when the wrapped function throws an error | |
| * @param onRejected Optional callback invoked when `fn` throws an error |
Is that fair?
| ); | ||
| }, | ||
| ttl: TTL, | ||
| onResolved() { |
There was a problem hiding this comment.
It would be nice if onResolved had a param where it received the successfully resolved aggregated referrer snapshot.
Then, the log info message generated here could also include the sum total count of aggregated referrers in the snapshot it just built.
This will be helpful for debugging.
| const indexingStatusResponse = c.var.indexingStatus.value; | ||
|
|
||
| if (indexingStatusResponse.responseCode === IndexingStatusResponseCodes.Error) { | ||
| return c.json( |
There was a problem hiding this comment.
I don't understand how it's possible to completely remove the logic for generating a API response with an error for the case that the registrar actions API is not available because no cache for the indexing status has ever been successfully built.
There was a problem hiding this comment.
There's one if clause earlier in this scope, that checks for c.var.indexingStatus.isRejected. When it's true it means the Indexing Status has never been fetched successfully.
| // reject response with 'error' responseCode | ||
| if (response.responseCode === IndexingStatusResponseCodes.Error) { | ||
| throw new Error( | ||
| "Received Indexing Status response with 'error' responseCode which will not be cached.", |
There was a problem hiding this comment.
| "Received Indexing Status response with 'error' responseCode which will not be cached.", | |
| "Received Indexing Status response with 'error' responseCode. The cached indexing status snapshot (if any) will not be updated.", |
| fn: async () => | ||
| client.indexingStatus().then((response) => { | ||
| // reject response with 'error' responseCode | ||
| if (response.responseCode === IndexingStatusResponseCodes.Error) { |
There was a problem hiding this comment.
Rather than checking for error, better to check for not ok.
| onRejected(reason) { | ||
| logger.error( | ||
| reason, | ||
| "Unable to fetch current indexing status. All fetch attempts have failed since service startup and no cached status is available. This may indicate the ENSIndexer service is unreachable or not responding.", |
There was a problem hiding this comment.
I'm getting confused. This error message is specifically for the case that ALL fetch attempts have failed. But as I understand, onRejected can be called after a fetch attempt was successful in initializing the cache.
| // handle both success and failure cases, including error details. | ||
| const indexingStatus = await pReflect( | ||
| cachedIndexingStatus !== null | ||
| ? Promise.resolve(cachedIndexingStatus) |
There was a problem hiding this comment.
Should this resolve a newly generated projection of the snapshot stored in cachedIndexingStatus?
Goal: downstream request handlers automatically get a freshly generated projection as of the current time.
If not relevant, no worries.
There was a problem hiding this comment.
Will take care of that 👍
| const indexingStatus = await pReflect( | ||
| cachedIndexingStatus !== null | ||
| ? Promise.resolve(cachedIndexingStatus) | ||
| : Promise.reject(new Error("Unable to fetch current indexing status.")), |
There was a problem hiding this comment.
Where do we generate the JSON in the API response for the case that we need to return an API response error because the indexing status cache has never been able to be successfully built?
There was a problem hiding this comment.
The JSON response is generated inside app.get("/indexing-status", async (c) => {}) handler in apps/ensapi/src/handlers/ensnode-api.ts file.
Document how `c.var.indexingStatus` can either represent 1) the realtime projection based on the last fetched `IndexingStatusResponseOk` value, or 2) the error while fetching the indexing status. Also, drops the `onResolved` and `onRejected` callback functions from `staleWhileRevalidate` function.
tk-o
force-pushed
the
fix/1200-ensapi-swr-use-native-promises
branch
from
5aa879b to
fdf3b0c
Compare
Co-authored-by: Tomasz Kopacki <tomasz@kopacki.net>
…se-native-promises
2 participants
Please review this PR with Hide whitespaces option on.
Suggested review order:
staleWhileRevalidatefunction input type).packages/ensnode-sdk/src/shared/cache.tsapps/ensapi/src/middleware/indexing-status.middleware.tsdocumented ideas and guarantees aroundc.var.indexingStatusvalue, and hownullbeing the cached value represents the state when indexing status has never been fetched successfully. Also, when the cached indexing status is available, thec.var.indexingStatusvalue represents the realtime indexing status projection that was created at the time of the request.apps/ensapi/src/middleware/aggregated-referrer-snapshot-cache.middleware.tsupdated thefnfetcher function to log a message for each time the aggregated referrer snapshot is built successfully.apps/ensapi/src/middleware/is-realtime.middleware.tsdropped the obsolete logic asc.var.indexingStatuscan never represent a response withresponseCode: IndexingStatusResponseCodes.Error.apps/ensapi/src/lib/subgraph/indexing-status-to-subgraph-meta.tssimplified the code according to the new guarantees ofc.var.indexingStatusvalue, thec.var.indexingStatuscan never represent a response withresponseCode: IndexingStatusResponseCodes.Error.apps/ensapi/src/lib/middleware/require-registrar-actions-plugins..middleware.tssimplified the code according to the new guarantees ofc.var.indexingStatusvalue, thec.var.indexingStatuscan never represent a response withresponseCode: IndexingStatusResponseCodes.Error.apps/ensapi/src/handlers/ensnode-api.tsupdated code docs, added extra log message.Context
This PR fixes an issue where the SWR cache query function would return
pReflectresult type, and not native promise. This caused the rejected promise to be seen as a correctly resolved one on the SWR cache level:The SWR cache requires the query
fnto return a native Promise object, so we cannot wrap the returned value withpReflect. On the other hand, in order to maintain the downstream logic which relies onpReflectresult type, we still usepReflect, but this time on the middleware level, and not on the SWR cache queryfnlevel.This PR updates the
c.var.IndexingStatusobject type fromIndexingStatusResponsetoRealtimeIndexingStatusProjection. It means that all downstream request handlers won't need to check theresponseCodeof the cached response. Only the OK responses can get cached. The Error ones are discarded (with an appropriate error log).