Conversation
Yaros1ove
requested review from
dgaponov,
imsitnikov and
stepanenkoxx
as code owners
src/api/server.ts
Outdated
| const libData = await this.libsCache[id].getData(); | ||
|
|
||
| return this.libsByIdCache[id].lib; | ||
| if (!libData) { | ||
| throw new Error(`Can't find lib with id – ${id}`); | ||
| } |
There was a problem hiding this comment.
I guess that if there will be no lib found by id we will get error "cannot read properties of undefined" while calling .getData() ?
| if (!content) { | ||
| throw new Error(`Can't find README for ${cacheKey}`); | ||
| } |
There was a problem hiding this comment.
same as in comment above
There was a problem hiding this comment.
src/api/cache-query.ts
Outdated
| return this._data as Data; | ||
| } | ||
|
|
||
| await this.revalidate(); |
There was a problem hiding this comment.
Am I correct in understanding that now, if the cache is outdated, we'll wait for it to update before serving the data?
Previously, we'd serve the data from the cache immediately and update the cache in parallel.
There was a problem hiding this comment.
I added immediateResponse parameter to getData function that is true on default
src/api/cache-query.ts
Outdated
| changeAutoRevalidate(newAutoRevalidate: boolean) { | ||
| if (this._autoRevalidateInterval) { | ||
| clearInterval(this._autoRevalidateInterval); | ||
| } | ||
|
|
||
| this._autoRevalidate = newAutoRevalidate; | ||
|
|
||
| if (newAutoRevalidate) { | ||
| this._autoRevalidateInterval = setInterval(() => { | ||
| this.revalidate(); | ||
| }, this._ttl); | ||
| } else { | ||
| this._autoRevalidateInterval = null; | ||
| } | ||
| } | ||
|
|
||
| clear() { | ||
| this._data = null; | ||
| this._cacheTimestamp = null; | ||
| this._state = 'stale'; | ||
| } | ||
|
|
||
| reset() { | ||
| this._data = null; | ||
| this._cacheTimestamp = null; | ||
| this._state = 'initial'; | ||
|
|
||
| this.changeAutoRevalidate(false); | ||
| } | ||
|
|
||
| changeTTL(newTTL: CacheTTL) { | ||
| this._ttl = CacheQuery.calculateTTLms(newTTL) || 0; | ||
|
|
||
| this.changeAutoRevalidate(this._autoRevalidate); | ||
| } |
There was a problem hiding this comment.
I think it's better to remove methods that are not needed yet
There was a problem hiding this comment.
I also have a question
Should I use FinalizationRegistry to clean up interval that auto revalidates cache in case of destroying query class instance?
There was a problem hiding this comment.
We seem to have an instance of cache class that exists for the entire life of the application, so I don't think we should bother with it now
I'd even remove autoRevalidate for now, since we're not using it right now :)
src/api/cache-query.ts
Outdated
| (ttl?.seconds ?? 0) * 1000 + | ||
| (ttl?.minutes ?? 0) * 60_000 + | ||
| (ttl?.hours ?? 0) * 3_600_000 + | ||
| (ttl?.days ?? 0) * 86_400_000 || 0 |
There was a problem hiding this comment.
Maybe its better to use || Infinity here?
So cache without ttl will be cache without updating
Now it will be cache that updates on every call
src/api/cache-query.ts
Outdated
| if (this._cacheTimestamp === null || this._data === null) { | ||
| this._state = 'stale'; | ||
|
|
||
| return this._state; | ||
| } |
There was a problem hiding this comment.
I guess we need to remove this branch
_cacheTimestamp could not be null in state fresh
And if the data in the "fresh" state is null, doesn't that mean that's what the query returned null and we don't need to update data?
src/api/cache-query.ts
Outdated
| return this._data; | ||
| } | ||
|
|
||
| return this._currentQueryPromise; |
There was a problem hiding this comment.
_currentQueryPromise could be null, because of this code
finally {
this._currentQueryPromise = null;
}
so i think it's better to make here something like
await this._currentQueryPromise;
return this._data;
There was a problem hiding this comment.
I'm not entirely sure what the problem might be here, but I would fix it
There was a problem hiding this comment.
I removed unused auto revalidation
And added await to currentQueryPromise
2 participants
No description provided.