Emit vuexPersistStateRestored event when async store replaced (addresses #15)

[morphatic]

Addresses issue raised in issue 15 by @rulrok. Also addresses #56. Supersedes #107.

This PR contains updated index.ts as well as README.md to update the documentation on how to use the new event. New functionality is tested in test/async-plugin-emits-restored.spec.ts.

As [noted in the docs](https://github.com/championswimmer/vuex-persist#note-on-localforage and-async-stores), the store is not immediately restored from async stores like localForage. This can have the unfortunate side effect of overwriting mutations to the store that happen before vuex-persist has a chance to do its thing. In strict mode, you can create a plugin to subscribe to RESTORE_MUTATION so that you tell your app to wait until the state has been restored before committing any further mutations. (Issue #15 demonstrates how to write such a plugin.) However, since you should turn strict mode off in production, and since vuex doesn't currently provide any kind of notification when replaceState() has been called, this PR would cause vuex-persist to emit a vuexPersistStateRestored event and also set a vuexPersistStateRestored flag to let you know the state has been restored and that it is now safe to commit any mutations that modify the stored state.

Here's an example of a beforeEach() hook in vuex-router that will cause your app to wait for vuex-persist to restore the state before taking any further actions:

// in src/router.js
import Vue from 'vue'
import Router from 'vue-router'
import { store } from '@/store' // ...or wherever your `vuex` store is defined

Vue.use(Router)

const router = new Router({
  // define your router as you normally would
})

const waitForStorageToBeReady = (to, from, next) => {
  if (!store._vm.$root.$data['vuexPersistStateRestored']) {
    store._vm.$root.$on('vuexPersistStateRestored', () => {
      next()
    })
  } else {
    next()
  }
}
router.beforeEach(waitForStorageToBeReady)

export default router

Note: store._vm.$root.$data['vuexPersistStateRestored'] will be undefined and therefore "falsy" prior to being set to true by the vuex-persist plugin. There should be no need to explicitly give it a value at any point.

[codecov]

Codecov Report

Merging #118 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #118   +/-   ##
=======================================
  Coverage   83.47%   83.47%           
=======================================
  Files           5        5           
  Lines         115      115           
  Branches       38       38           
=======================================
  Hits           96       96           
  Misses          7        7           
  Partials       12       12

Impacted Files	Coverage Δ
src/index.ts	77.77% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52517d1...eaf3806. Read the comment docs.

[morphatic]

For anyone who may be waiting on this PR to be merged, in the meantime, you can install a patched version of this package directly from my fork as follows:

npm install --save github:morphatic/vuex-persist

Or, if you had already installed the package, but want the patched version, update the relevant line from your package.json file as follows and then run npm install from the command line:

// package.json
{
  dependencies: {
    // ... other stuff
    "vuex-persist": "github:morphatic/vuex-persist"
  }
}

Hth

[hotdogee]

May I suggest a promise based implementation for cleaner code.

// index.ts
/**
 * Async version of plugin
 * @param {Store<S>} store
 */
this.plugin = (store: Store<S>) => {
  (store as any).restored = ((this.restoreState(this.key, this.storage)) as Promise<S>).then((savedState) => {
    /**
     * If in strict mode, do only via mutation
     */
    if (this.strictMode) {
      store.commit('RESTORE_MUTATION', savedState)
    } else {
      store.replaceState(merge(store.state, savedState || {}))
    }
    this.subscriber(store)((mutation: MutationPayload, state: S) => {
      if (this.filter(mutation)) {
        this._mutex.enqueue(
          this.saveState(this.key, this.reducer(state), this.storage) as Promise<void>
        )
      }
    })
    this.subscribed = true
  })
}

The waitForStorageToBeReady function in your example then becomes:

// in src/router.js
const waitForStorageToBeReady = async (to, from, next) => {
  await store.restored
  next()
}

Thanks

[morphatic]

@hotdogee I agree your way is much cleaner! I tried making the change in my fork, wrote a test for it, and it appears to work perfectly.

There might be one issue it doesn't address, but perhaps you have an equally elegant workaround, or it could be I'm not thinking through it clearly.

The issue is that the beforeEach() hook is called every time someone navigates to a new route, but the store only needs to be refreshed when there is an actual page refresh or the app is opened in a new window. I set the vuexPersistStateRestored flag so that I would know if the state had already been restored or not, and could therefore be skipped in the hook.

What happens when you hit the router after the initial restore and you hit the await store.restored for the second time? If the promise has already resolved, does it stay resolved? Is it possible for the app to get stuck waiting for await store.restored to resolve on route requests where there the store is not being restored? Does this make sense?

If it works, I'll update the PR to use your approach. I like your way much better. Wish I'd thought of it!!

[hotdogee]

@morphatic Awesome, I'm glad you could make it work.

What happens when you hit the router after the initial restore and you hit the await store.restored for the second time? If the promise has already resolved, does it stay resolved?

The plugin Promise is created and assigned to the restored key on the store instance during new Vuex.Store(), as long as store references the existing Store instance, store.restored will always reference the same Promise. The Promise will resolve after the last line in the then function executes (currently this.subscribed = true), and will stay resolved for any future await store.restored, in the resolved state await store.restored is equivalent to checking a truthy vuexPersistStateRestored flag and will run the next statement immediately.

Is it possible for the app to get stuck waiting for await store.restored to resolve on route requests where there the store is not being restored?

Yes, it is possible to get stuck, but it is the same case if $emit() doesn't get run and also gets stuck. One solution is to add a timeout and handle the case where the store was never successfully restored.

// in src/router.js
const waitForStorageToBeReady = async (to, from, next) => {
  try {
    await Promise.race([
      store.restored,
      new Promise((resolve, reject) => {
        const ms = 3000
        const id = setTimeout(() => {
          clearTimeout(id)
          reject(new Error(`Timed out in ${ms} ms.`))
        }, ms)
      })
    ])
  } catch (error) {
    // handle the case where the store was never successfully restored
  }
  next()
}

Warning: sorry, I did not have a computer on hand to actually test the code above at this moment.

Thanks~

[morphatic]

@hotdogee Thank you for the fantastic suggestions! I went ahead and revised this PR to use your approach instead of the one that I had before. In my use case, there are some things I need to do only after the first restore. Here's what I found out:

// I tried this first, but it did NOT do what I intended...
const waitForStorageToBeReady = async (to, from, next) => {
  await store.restored.then(
    () => {
      // this code will run on EVERY route request
    }
  )
  next()
}

I did not want the post-restore code to run on every request, so I did something like this:

let previouslyRestored = false
const waitForStorageToBeReady = async (to, from, next) => {
  await store.restored
  if (!previouslyRestored) {
    // do the things you want to do only once after the store is restored
    previouslyRestored = true
  }
  next()
}

⚠️ Caution

In the process of testing this I discovered that if your store is in strict mode (store.strict === true) and vuex-persist is NOT in strict mode (strictMode === false) the plugin will silently fail because vuex doesn't allow you to mutate the state outside of mutations, but for some reason I never got a warning either in the console or in devtools. I spent a couple of hours being stuck on this before I figured out what was going on. 😓

As a result, I opened issue #121 to suggest that vuex-persist should ALWAYS use RESTORE_MUTATION. This is actually more in keeping with the official vuex docs, and it would actually make this PR unnecessary since you could always just subscribe to the mutation.

In the meantime, you can install an updated version of the plugin from the master branch of my fork as described above which has the changes you suggested.

Thanks!!!

[championswimmer]

Would prefer something like this #121 than this PR, if you want to do that instead, make a PR for that please 😇

[morficus]

So this PR actually makes v2.1.0 unusable. like @morphatic mentioned... there is a silent failure when the store is set to strict but the plugin is not. this results in the app crashing silently and leaving everything unusable. This is particularly troubling because tools like Nuxt or a project generated by the Vue CLI will put the store in to strict-mode when running in non-production.

@championswimmer Can you please update the documentation to mention that if someone uses the router.beforeEach trick.... then then plugin and the store MUST both be in the same "strict" state.

[championswimmer]

Send a PR with an updated README please ? :)

…

On Thu, 12 Sep, 2019, 5:04 AM Maurice Williams, ***@***.***> wrote: So this PR actually makes v2.1.0 unusable. like @morphatic <https://github.com/morphatic> mentioned... there is a silent failure when the store is set to strict but the plugin is not. this results in the app crashing silently and leaving everything unusable. @championswimmer <https://github.com/championswimmer> Can you please update the documentation to mention that if someone uses the router.beforeEach trick.... then then plugin and the store MUST both be in the same "strict" state. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#118?email_source=notifications&email_token=AAKD7SXILRNHMVFKRLDLYDTQJF6CDA5CNFSM4HO3FTOKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6QGJ5A#issuecomment-530605300>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKD7SUUD6RRR6QUMNT55ADQJF6CDANCNFSM4HO3FTOA> .