1
\$\begingroup\$

I'm working on building a Browser storage handler for my single page application. The idea is that you make an API call and the API calls the storage handler which either returns the local/session storage or if it doesn't exist it calls the actual web service. The StorageHandler uses a Proxy to first check if the browser has local and session storage enabled. The "callback" is really just an Axios instance with a base url and some headers attached. TBH I'm not sure this is even remotely a good idea or if it should just be handled in the actual API since I've read wrapping a Promise with another Promise is a bad idea and I don't have much experience with proxies.

const StorageHandler = {
    TestedStorage: false,
    StorageEnabled: false,
    TestStorage() {
        this.TestedStorage = true;
        this.StorageEnabled = this.TestLocal();
        return this.StorageEnabled;
    },
    TestLocal() {
        if (typeof window.localStorage !== 'undefined') {
            try {
                const test = 'TEST';
                window.localStorage.setItem(test, test);
                window.localStorage.removeItem(test);
                return true;
            } catch (e) {
                return false;
            }
        }
        return false;
    },
    ClearLocal() {
        window.localStorage.clear();
    },
    UpdateLocalWithObj(url, obj) {
        window.localStorage.setItem(url, JSON.stringify(obj));
    },
    // Gets from local storage or calls api, burst refers to cache bursting
    GetOrSetLocal(url, callback, burst = false) {
        return new Promise((resolve, reject) => {
            let result;
            if (this.StorageEnabled && burst === false) {
                result = window.localStorage.getItem(url);

                if (result !== null && result !== 'null') {
                    resolve(JSON.parse(result));
                }
            }

            callback.get(url)
            .then((response) => {
                result = response.data;
                if (this.StorageEnabled) {
                    window.localStorage.setItem(url, JSON.stringify(result));
                }
                resolve(result);
            })
            .catch((error) => { reject(error); });
        });
    },
    ....
};

const ProxyHandler = {
    get(target, key) {
        if (target.TestedStorage === false) {
            target.TestStorage();
        }
        return target[key];
    },
};

const proxy = new Proxy(StorageHandler, ProxyHandler);

export default proxy;
\$\endgroup\$
1
\$\begingroup\$

You are correct that there is no need to wrap this with a promise. As it is, there are a few problems with the GetOrSetLocal function.

  1. The function will always try to fetch the values from the server, even if it is cached locally.
  2. The function's name is misleading, when first reading the code I assumed the function could be used to set a value in storage - not just retrieve values. I'm not really sure what to suggest for a name...
  3. I would recommend using something other than callback for the Axios instance as it is decidedly not a callback. It might be best to just call it axios.
  4. burst would be better named ignoreCache to more clearly indicate it's purpose.

If I was unable to use async/await, here is how I would write this function:

const StorageHandler = {
    ...,
    GetFromCache(url) {
        if (!this.StorageEnabled) return null
        return JSON.parse(localStorage.getItem(url))
    },
    GetOrSetLocal(url, axios, ignoreCache = false) {
        if (!ignoreCache) {
            return new Promise(resolve => {
                const cached = this.GetFromCache(url)
                resolve(cached === null ? this.GetOrSetLocal(url, axios, true) : cached)
            })
        }

        return axios.get(url)
            .then(response => {
                if (this.StorageEnabled) {
                    localStorage.setItem(url, JSON.stringify(response.data))
                }
                return response.data
            })
    },
}

Alternatively, if you are able to use async/await, I would prefer this version.

const StorageHandler = {
    ...,
    async GetOrSetLocal2(url, axios, ignoreCache = false) {
        if (!ignoreCache) {
            const cached = this.GetFromCache(url)
            return cached === null ? this.GetOrSetLocal2(url, axios, true) : cached
        }

        const { data } = await axios.get(url)

        if (this.StorageEnabled) {
            window.localStorage.setItem(url, JSON.stringify(data))
        }

        return data
    },
}

Another comment: I tend to avoid using proxies as I find they quickly multiply and end up making debugging a royal pain. It might be better to define StorageEnabled as a getter.

const StorageHandler = {
    TestedStorage: false,
    _StorageEnabled: false,
    get StorageEnabled() {
        if (!this.TestedStorage) {
            this.TestStorage()
        }
        return this._StorageEnabled
    }
}

And one more thing: It is probably better to implement caching with response headers instead of saving responses in localStorage. Since this is a get api, you can use headers to tell user's browsers to cache the responses for the appropriate amount of time and you get faster responses automatically, without introducing more JS. (Unless the response is out of your control of course - in which case this is probably a good idea.)

\$\endgroup\$
  • 1
    \$\begingroup\$ Incredibly thorough. I completely overlooked getters for StorageEnabled. I ended up taking most of your advice. Also renamed the callback parameter http in case axios is ever swapped out. The response is indeed out of my control as far as cache headers go. \$\endgroup\$ – Steven B. Dec 26 '17 at 1:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.