Automattic / jetpack

Security, performance, marketing, and design tools — Jetpack is made by WordPress experts to make WP sites safer and faster, and help you grow your traffic.
https://jetpack.com/
Other
1.58k stars 799 forks source link

Instant Search: API cache fallback, timeout, localStorage #13751

Open gibrown opened 4 years ago

gibrown commented 4 years ago

It seems like we could greatly improve our search API handling for when there are errors:

    return fetch(
        `https://public-api.wordpress.com/rest/v1.3/sites/${ siteId }/search?${ queryString }`
    ).then( response => {
        if (response.ok) {
            return response;
        } else {
            const error = new Error( response.statusText );
            error.response = response;
            return Promise.reject( error );
        }
    } ).then( response => {
        const r = response.json();
        apiCache.put( key, r );
        return r;
    } ).catch( error => {
        const cachedOldVal = apiCache.get( key );
        if ( cachedOldVal ) {
            return cachedVal;
        }
        return error;
    } );
//Two layer in memory cache
// - 5 min TTL normally
// - 30 min to fall back on if we lose connectivity
//TODO: store cache data in the browser - esp for mobile
const apiCache = new Cache( 300 * 1000 );
const apiCacheFallback = new Cache( 1800 * 1000 );
gibrown commented 4 years ago

In addition to caching the previous results, we should cache what search queries the user has previously made and do something intelligent about displaying them in a drop down as the user starts typing. Eventually these could be mixed with auto correct/suggest from the api

jsnmoon commented 4 years ago

Currently we don't do anything about non-200 responses from the API. Can probably do something like this:

This is worth exploring. We'll probably want to notify the user about the connection issue in the interface as well.

I think we could have multiple layers to our api caching: That apiCacheFallback we should probably store in localStorage so that we can fallback to it even when losing network connectivity.

There's a danger that we'll bloat the browser storage if we don't cap it somehow. Thoughts on this, @bluefuton?

Implement some sort of a request timeout

I think this is worth exploring. Would we fallback to the 30-minute cache if the network request takes more than a second?

We need to handle canceling in transit requests

Also agreed. I previously explored using the AbortController with the Fetch API but found the browser support lackluster. We'll probably want to explore rolling out our own solution.

jsnmoon commented 4 years ago

Alternatively, this might be the final push necessary for us to start using axios for Instant Search.

gibrown commented 4 years ago

Would we fallback to the 30-minute cache if the network request takes more than a second?

Ya, I think falling back to the 30-minute cache would make sense here. Maybe this longer cache isn't even time limited but is space limited on the device. LRU presumably? I agree we should have some message to the user that they are seeing old results due to connection problems.

Does seem like it would be cool to find a library that implements a lot of these things for us.

Probably not helpful, but we did try and implement some of this in the last push for instant search (5 years ago!): https://github.com/Automattic/jetpack-instant-search/tree/master/client/searcher

jsnmoon commented 4 years ago

we should probably store [apiCacheFallback] in localStorage so that we can fallback to it even when losing network connectivity. If that is the case then we should probably keep it around longer (or indefinitely).

Do we need to worry about privacy issues for caching network requests in browser storage? cc @bperson @bluefuton

stale[bot] commented 4 years ago

This issue has been marked as stale. This happened because:

No further action is needed. But it's worth checking if this ticket has clear reproduction steps and it is still reproducible. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.