Skip to content

Conversation

@addyosmani
Copy link
Collaborator

This change adds support for a read-through caching adapter on top of the Firebase API that fixes #25. It enables support for caching comment-threads and other non-UI content responses being returned from Firebase for react-hn views. We use localForage/IDB for the caching layer, falling back to localStorage.

This change uses https://github.com/GoogleChrome/firetruck (not yet public, but going through Google open-source releasing atm). Both @insin and @NekR should have access to look at what's there atm. It's not particularly fancy. The adapter doesn't yet support set() operations but since this project is mostly just reading HN data, I think we should be fine.

DevTools > Resources > IndexedDB showing records cached:

screen shot 2016-04-25 at 14 00 02

Here's a demo of a built version of the app still being able to view comments when the network is killed:
📹 https://www.youtube.com/watch?v=Nz_f9Gfii6w&feature=youtu.be

There are quite a few perf improvements that can be made to Firetruck (we start to hit slow reads when you're looking at large (e.g 300+) comment threads and could optimize bottlenecks like JSON.stringify(), but I can look at that next. Consider this a better-than-what-we-have-now addition :)

Things we can/should do better in future

  • Avoid filling up IndexedDB by having some sort of cache-expiration model for older cached entries
  • Properly catch attempts to set that won't be cached and provide a meaningful message. Even better would be scheduling writes, but again, React HN doesn't really need this
  • Be smarter about when we read from IndexedDB vs. re-fetching from the network

This change adds support for https://github.com/GoogleChrome/firetruck,
a simple POC read-through caching adapter on top of the Firebase API.
@NekR
Copy link
Collaborator

NekR commented Apr 25, 2016

I'll review it a bit later today.

Meanwhile, I have been thinking for a while about using Cache Storage API as a simple key-value storage, like an async super set of localStorage. I know that localForage solves exactly that problem, but it's soft of big library (26kb min is huge in my mind). Anyway, just thoughts at a moment.

@addyosmani
Copy link
Collaborator Author

Thanks @NekR.

Given we're mostly trying to get this working in evergreen browsers, it might be possible to switch to a more lightweight wrapper around IDB later (Jake mentioned IDB made more sense than the Cache API for this use-case, but I'll try digging up that convo).

@NekR
Copy link
Collaborator

NekR commented Apr 25, 2016

(Jake mentioned IDB made more sense than the Cache API for this use-case, but I'll try digging up that convo)

Probably. Cache Storage API just seems a bit easier. Btw, great to see firetruck coming out 👍

@NekR
Copy link
Collaborator

NekR commented Apr 26, 2016

I checked firetruck's code couple of times, complicated but seems fine. Although I am not a Firebase expert.

I didn't test project locally yet, but it works on video, so merging.

@NekR NekR merged commit dda8e09 into master Apr 26, 2016
@addyosmani
Copy link
Collaborator Author

As we eventually switched to a more complete offline mode implementation in #36 and moved away from using Firetruck, I'm linking up to the Firetruck implementation just for historical context: https://gist.github.com/addyosmani/bbc8c5c73dc4105292b397cfff68c073

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support caching comment-threads / page content offline

3 participants