i18next / i18next-localstorage-backend

This is a i18next cache layer to be used in the browser. It will load and cache resources from localStorage and can be used in combination with the chained backend.
MIT License
88 stars 43 forks source link

Implementing Default Versions to Clear Cache #23

Closed sonejishruti closed 4 years ago

sonejishruti commented 4 years ago

When trying to implement the default version feature I found a bug.

Repo Code: https://github.com/i18next/i18next-localstorage-backend/blob/master/i18nextLocalStorageBackend.js#L146

Inside the if statement the logic is such that it will never clear cache when version is updated. The current logic statues that if the time expires and the version is the same then clear cache. But, it does not address if the version is different clear cache. The logic should be: if ( local.i18nStamp && local.i18nStamp + this.options.expirationTime < nowMS || version !== local.i18nVersion)

Could this bug fix be made to the repo? Or advise if further details on my implementation are needed.

jamuhl commented 4 years ago

If I read it right the code says: When the cache is not expired and the version is still the same -> return the data from localStorage -> else return nothing which will trigger the next backend in the multiloadBackend (that data then be pushed back to localStorage)

LionOps commented 4 years ago

I worked with @sonejishruti trying to debug the behavior that we were seeing. When updating a version, the local storage was not being cleared and the updated strings were not being fetched.

var local = this.storage.getItem("".concat(this.options.prefix).concat(language, "-").concat(namespace));

        if (local) {
          local = JSON.parse(local);
          var version = this.getVersion(language);

          if ( // expiration field is mandatory, and should not be expired
          local.i18nStamp && local.i18nStamp + this.options.expirationTime > nowMS // there should be no language version set, or if it is, it should match the one in translation
          && version === local.i18nVersion) {
            delete local.i18nVersion;
            delete local.i18nStamp;
            return callback(null, local);
          }
        }

An assumption that we made was that the delete statements were necessary to have the next backend make the requests. When we made changes locally to the above code, things worked as expected.

The changes that we made were to negate the logic. local.i18nStamp && local.i18nStamp + this.options.expirationTime > nowMS && version === local.i18nVersion became local.i18nStamp && local.i18nStamp + this.options.expirationTime <= nowMS || version !== local.i18nVersion so that the conditional code was ran if either the current time was greater than the expiration (which we understand to mean that things expired) or if there was a version mismatch.

jamuhl commented 4 years ago

So to make the code more understandable...as your on the wrong path:

Success case (use the localstorage data): https://github.com/i18next/i18next-localstorage-backend/blob/master/src/index.js#L73

Do not use them: https://github.com/i18next/i18next-localstorage-backend/blob/master/src/index.js#L77

Chained backend logic (load from next backend): https://github.com/i18next/i18next-chained-backend/blob/master/src/index.js#L39

sonejishruti commented 4 years ago

We were able to figure out our mistake. Thank you for your time and help.