-
Notifications
You must be signed in to change notification settings - Fork 175
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
304 breaks local caching. #2050
Comments
Irrespective of the 304 issue — what's your goal here? That |
My specific goal when stumbling into it was making an offline capable web app (which you can reload in the browser and works because the data fetched is in the browser cache -- which works 💯 when the requests are cached but obviously fails when any request in the sequence falls out of the cache). More generally, this impacts the proposition of "because Electric is just HTTP and data loading is cached, it comes out of the local browser cache / local edge cache / any cache". So the idea that you can make resilient apps that tolerate downtime without having to implement actual local persistence because the requests are just naturally cached. And the idea that you can navigate back to a page you visited before and you won't get a loading spinner. In short:
|
ah ok — so what I think we want here is that a 304 should include cache-control headers to extend the original cache. Probably the same max-age as the original cache. Or whatever the dynamically set max-age is once we have #1447 to calculate that. The problem now is that original cache expires and we never extend it due to not setting a max-age on the 304s. That solves 1. For 2 — this seems like a client or end-user configuration problem — we do want fresh data by default and should only fall back on cached data if we know we're offline. There's no easy way to know we're offline or what the state of the cached data is w/o some sort of timeout on fetched responses, etc. But ultimately, I don't think we should expect offline to work with the raw browser http cache — we don't control it and can't introspect on it. We have no idea if the cache is complete, etc. I think if people want actual offline support then they need to use indexdb/sqlite/pglite/etc. which can control the cache and reason about it. |
I’m not sure re 2. The thing is that the ability of the browser to cache responses is very powerful and works for free for people out of the box. So it’s a strong incremental adoption thing that aligns with our positioning and differentiation. To flip the question around, what are the drivers for not caching data you’ve received? Why can’t we support caching responses for a decent length of time? The point about wanting fresh data: there’s nothing about keeping the data you have synced cached that precludes the client from constantly trying to fetch fresh data. That’s the current behaviour — it keeps trying to connect. When it comes to why sync not fetch, the basic affordance of “navigate back to a previously viewed route” -> “look ma no loading spinner” is a big deal. The beauty is that we can deliver it without any of the complexity or code changes required to implement a local persistent store. So it feels worthwhile eyeballing the caching duration and up-to-date stuff to see if we can achieve it. |
Most apps don't want to immediately render old data and then a bit later render fresh data. It looks glitchy. Ultimately it's the app developer who should choose how long to cache data for. So we can add that as an option to the http API. But keeping the default short is good as glitchy uis are annoying for most apps. |
Yes, I can see there’s less control with this just-replay-the-requests approach. I would love for there to be a best of all worlds solution — where the data is just there in the file cache and that doesn’t compromise UX. I wonder if there might be some cleverness to add a little more control back in, like ignoring all but the last up-to-date message when detecting requests served from file cache. I think my viewpoint is that once you bind a shape to a state variable with But that is the UX you’d get with a persisted shape, so if we can provide the same UX without having to implement persistence that would be 🔥 (and is actually how it already works, within the cache window). |
Just for context here's a little video of some current behaviour: Screen.Recording.2024-11-27.at.18.17.48.mp4 |
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
In
Electric.Plug.ServeShapePlug
, specifically here https://github.com/electric-sql/electric/blob/main/packages/sync-service/lib/electric/plug/serve_shape_plug.ex#L359, we check the etag of a request and return 304 if it matches:This doesn't set any caching headers. So ... the response comes back as
max-age=0, private, must-revalidate
:This breaks local caching in the browser, which breaks offline support for Electric apps. Looking at the request in the Chrome dev tools, it comes through as
200
rather than a304
(which is confusing, seems to be a Chromium bug https://issues.chromium.org/issues/40205097) but it is the 304 response and the cache-control header hasmax-age 0, must revalidate
, so isn't cached in the local file cache:I came across this because I noticed that it was breaking offline support:
To reproduce this you have to actually get the 304 request. This is returned when the browser re-fetches a URL after the local file cache has expired. So then re-fetching successfully when online gets a 304, which is then never cached so the offline support only works for the initial request.
Now ... the duration of the cache-control headers is very different for requests with content:
Vs requests that just have an
up-to-date
message:Unfortunately because the up-to-date message is needed to trigger rendering of the content, this means that the content that's been loaded and is cached locally can't be used because the client still needs an up-to-date in order to display it.
So:
The text was updated successfully, but these errors were encountered: