Discontinue search result caching

[tofumatt]

Right now we cache search results and return data already in state rather than making another API request. But as @kumar303 pointed out, this means we could run into a single-page-app state where we have stale data that requires a refresh to fix.

While cached data will improve load times it will also means stale pages. Either we should:

• stop caching data entirely and always perform an API request for search (I'm for this)
• set a time limit to hold cached data for (maybe a 60 second or so period where data is kept would be reasonable, but this would mean a world of hurt building cache invalidation which the API+browser should also already kinda take care of)

[Ryuno-Ki]

How do you cache search results? Isn't there an E-Tag headerfield which tells you for how long it is safe to cache it?

[tofumatt]

We cache search results inside the redux state of the addons-frontend app. This is about the frontend caching data, not about the server. Please see the link (https://github.com/mozilla/addons-frontend/pull/1398#discussion_r88944806) in my original comment.

[Ryuno-Ki]

To me it reads as you would cache the response of an API call, that is, an XHR to the server. In that response header the server should tell you for how long it is safe to cache the response content.

[tofumatt]

Yes, that is what I am saying. The browser should cache the call so our JS state caching is redundant or at least at the wrong place in the process.

[mstriemer]

I think search is only caching a single page, based on the reducer. This means if you perform another search it will clear the old search results (the add-on data is still loaded but possibly not accessible without another search).

[tofumatt]

Ah true. In that case it’s a fair extra amount of code + tests for gain, as someone would need to dispatch the same action twice to load data from the cache. I guess if they hit enter on the search box, on page 1, with the same query it’d do it…

Maybe we don’t need to care about this then.

[kumar303]

While cached data will improve load times...

Does it really though? The API request will already be cached by the browser. If the back button hits the API again it might still be snappy. This kind of caching feels like something we should handle on the API server, not in the frontend app.

stop caching data entirely and always perform an API request for search

Yes, I think this is a good plan. Note that it will still be cached at the browser request level.

set a time limit to hold cached data for

Why do we need to still cache it in the frontend app? It seems like complexity that will only introduce new bugs.

[muffinresearch]

Big 👍 for keeping it simple.

[mstriemer]

The line referenced in https://github.com/mozilla/addons-frontend/pull/1398#discussion_r88944806 is caching one page of search results because that function will get called every time props change and after the initial page render (I believe).

If we don't cache that then I think we're going to be hitting the API on the server to include the content in the first render and then again when our JS finishes loading. We do this sort of caching anywhere we are blocking on the server for page render.

Removing this caching may prevent stale data but I don't think it's the right approach. What caching are we trying to prevent exactly?

[kumar303]

That line was pointing to a callback hooked up in asyncConnect. Isn't that only called when the component first renders?

Every time a user navigates to the search results (via back button, for example), I think the component should make an API request to populate state data. (This API request/response may be cached by the browser.)

If the component gets re-rendered with new props or for unrelated state changes then, yes, it wouldn't make sense to perform an API request.

Maybe it can be solved for those rules ^ ?

[mstriemer]

So I tried poking around the CategoryPage component and it looks like asyncConnect doesn't actually cause an extra request on page load. I find this surprising though since it just boils down to a connect call in the library. I wonder if we're just getting a bit lucky with this not causing the request on initial page load. I know redux checks the state when it rehydrates on the client so I was expecting this to cause a request. Perhaps the chain is ended early because this isn't the root component and props didn't change higher up.

It does cause a refresh of the data when using the back button, so if you navigate to http://localhost:3000/en-US/firefox/extensions/bookmarks/ then click an add-on and use the back button it takes a bit for the page to actually update because it's request data from the server. This isn't actually what the browser does on back button natively, though. It will ignore the header it was provided and serve a full HTML page from cache if it has it, this is why if you're on a slow connection and do a web search, click through and hit the back button the results are loaded right away.

[Ryuno-Ki]

Is there an event listener for pageshow? Because in my experience window.onload won't get fired if you traverse the history using the back button.

@mstriemer Is the request fired if you refresh the page? Usually that would trigger API calls.

[mstriemer]

I don't think we're listening to pageshow but we are managing the history using react-router so it knows when push/pop state events happen. Reloading the page would trigger an API call on the server, the client would not get a browser cache version of this.

[kumar303]

[the browser] will ignore the header it was provided and serve a full HTML page from cache

Huh, wow. As the title of this article suggests, I did not know how the back button worked! My bad. https://madhatted.com/2013/6/16/you-do-not-understand-browser-history

So I guess we need to keep caching to make the back behavior consistent with the browser.

[Ryuno-Ki]

Hence I asked for pageshow event listeners :)

[tofumatt]

I think with the move to sagas this is effectively done. We don't tend to cache results at all anymore. Closing.