Add disk cache store #949
Add disk cache store #949
Conversation
afcapel
force-pushed
the
http-cache-store
branch
from
5666de6
to
8463af2
Compare
There was a problem hiding this comment.
This looks great! 🎉 It's a very good idea, and the implementation is simple and clear 👏
I left a couple of questions but it's all just minor curiosity stuff.
Also, as we discussed elsewhere, it'll be worth us looking at ways to keep more of the cache for longer, rather than clear it after every mutation. I reckon there's lot of potential in having a longer-lived cache once this is in place. But that seems better explored separately.
|
|
||
| async clear() { | ||
| const keys = await this.storage.keys() | ||
| await Promise.all(keys.map((key) => this.storage.delete(key))) |
There was a problem hiding this comment.
I wonder if there's a point where it's quicker to delete the cache itself, rather than the individual items. Perhaps it's very unlikely to come up in any case. But I wondered if that's something you looked at?
There was a problem hiding this comment.
The cache API only has a method to delete individual keys https://developer.mozilla.org/en-US/docs/Web/API/Cache
There was a problem hiding this comment.
You can delete the Cache instance though, instead of deleting the individual items that it contains: https://developer.mozilla.org/en-US/docs/Web/API/CacheStorage/delete
There was a problem hiding this comment.
Ah, I missed that 👍
I think deleting the keys is ok, though. Don't think there will be a performance penalty, and because the operations are async there's a small chance of race condition if we delete the cache and reopen it again when there are other operations inflight.
| await page.goto(path) | ||
| }) | ||
|
|
||
| test("test stores pages in the disk cache", async ({ page }) => { |
There was a problem hiding this comment.
I think this does verify that the cache gets populated, but is it also worth testing that those cached entries definitely get used? Maybe the test could listen for the turbo:render event for the cached version when following a link?
There was a problem hiding this comment.
As discussed offline, it'd be nice to have a test checking the cache (we don't have any at the moment), but for some reason this doesn't work in the test env. Will have to punt this for now.
afcapel
force-pushed
the
http-cache-store
branch
from
4e91716
to
f365321
Compare
afcapel
force-pushed
the
http-cache-store
branch
from
f365321
to
fa3ce2e
Compare
This commit extends the Turbo cache API to allow for the use of different cache stores. The default store is still the in-memory store, but a new persistent cache store is now available. The disk store uses the [Cache API](https://developer.mozilla.org/en-US/docs/Web/API/CacheStorage) to store and retrieve snapshots from the browser's cache. This allows for the snapshots to be persisted across different tabs, page reloads and even browser restarts. The disk store is not enabled by default. To enable it, you need to set the `Turbo.cache.store` property to `"disk"`. ```js Turbo.cache.store = "disk" ``` This is also a stepping stone to implement offline support with Service Workers. With a Service Worker in place, and the disk cache store enabled, we can still serve cached snapshots even when the browser is offline.
afcapel
force-pushed
the
http-cache-store
branch
from
fa3ce2e
to
f63f0a3
Compare
This commit fixes a bug where navigating back after submitting a POST form would not show the previous page. The bug was introduced in #949. The new cache API is async since it needs to access CacheStorage which is async. In that PR, getCachedSnapshot was changed to be async, but hasCachedSnapshot was not changed. This meant that getCachedSnapshot would always return a promise, which is truthy, and so hasCachedSnapshot which just checked that the return value was not null would always return true. The bug would show up when you tried to navigate back to a page after a post form submission. The form submission would clear the cache, but the restoration visit would think it had a cached snapshot and so would not issue a request. This meant that the page would not be restored and nothing would happen. There's a new test in navigation_tests.js that reproduces this bug. The fix is to make hasCachedSnapshot async and await it in the places where it is used.
The cache interface changed in #949 Since the cache can be loaded from disk using the browser Cache API, it is possible that the cache snapshot will be loaded asynchronously. This means that the calls to load cache snapshots need to be awaited. We also changed visit.hasCachedSnapshot() to be async and return a Promise. However, [hasCachedSnapshot is used in the iOS adapter](https://github.com/hotwired/turbo-ios/blob/c476bac66f260adbfe930ed9a151e7637973ff99/Source/WebView/turbo.js#L119) and serialized into and data object we send to the native side via postMessage. When postMessagereceives a Promise instead of a boolean value it fails with a DataCloneError since it can't serialize the Promise. This commit moves the cache snapshot loading to Visit#start() and stores the result in a cachedSnapshot property. This allows us to keep the hasCachedSnapshot() method synchronous and return a boolean value. It means that Visit.start is now async, but I haven't found any callers that rely on it being synchronous.
* Move cache snapshot loading to Visit#start() The cache interface changed in #949 Since the cache can be loaded from disk using the browser Cache API, it is possible that the cache snapshot will be loaded asynchronously. This means that the calls to load cache snapshots need to be awaited. We also changed visit.hasCachedSnapshot() to be async and return a Promise. However, [hasCachedSnapshot is used in the iOS adapter](https://github.com/hotwired/turbo-ios/blob/c476bac66f260adbfe930ed9a151e7637973ff99/Source/WebView/turbo.js#L119) and serialized into and data object we send to the native side via postMessage. When postMessagereceives a Promise instead of a boolean value it fails with a DataCloneError since it can't serialize the Promise. This commit moves the cache snapshot loading to Visit#start() and stores the result in a cachedSnapshot property. This allows us to keep the hasCachedSnapshot() method synchronous and return a boolean value. It means that Visit.start is now async, but I haven't found any callers that rely on it being synchronous. * Explicitly test that visit.hasCachedSnapshot() returns a boolean, which is what the native adapters expect * Remove the "test " prefix from the native adapter unit tests, since other tests were recently updated to remove the prefix --------- Co-authored-by: Jay Ohms <jay.ohms@gmail.com>
* Revert "Move cache snapshot loading to Visit#start() (#1056)" This reverts commit e07639f. * Revert "Fix back navigation after POST form submission (#1014)" This reverts commit c207f5b. * Revert "Add disk cache store (#949)" This reverts commit f86a376. * Remove redudant test prefix * Bring back test Orginally added by @jayohms in 60cfbee
This commit extends the Turbo cache API to allow for the use of different cache stores. The default store is still the in-memory store, but a new persistent cache store is now available.
The disk store uses the Cache API to store and retrieve snapshots from the browser's cache. This allows for the snapshots to be persisted across different tabs, page reloads and even browser restarts.
The disk store is not enabled by default. To enable it, you need to set the
Turbo.cache.storeproperty to"disk".This is also a stepping stone to implement offline support with Service Workers. With a Service Worker in place, and the disk cache store enabled, we can still serve cached snapshots even when the browser is offline.