Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit vuexPersistStateRestored when state replaced for async stores #107

Closed
wants to merge 5 commits into from
Closed

Conversation

morphatic
Copy link

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

This PR contains updated indext.ts as well as README.md to update the documentation on how to use the new event.

As noted in the docs, the store is not immediately restored from asnyc 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 1: 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.

Note 2: My linter automatically added in some spaces in the import statements at the top of the module, and also reformatted the indentation of restoreState() and saveState(). I tried to revert and reapply my changes with the linter off so that this wouldn't happen, but it seemed to be following the rules already expressed in tslint.json so I gave up and left it as is. I don't make a habit of reformatting other people's code. ;)

@codecov
Copy link

codecov bot commented Apr 7, 2019

Codecov Report

Merging #107 into master will increase coverage by 0.28%.
The diff coverage is 62.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #107      +/-   ##
==========================================
+ Coverage   83.47%   83.76%   +0.28%     
==========================================
  Files           5        5              
  Lines         115      117       +2     
  Branches       38       38              
==========================================
+ Hits           96       98       +2     
  Misses          7        7              
  Partials       12       12
Impacted Files Coverage Δ
src/index.ts 78.37% <62.5%> (+0.6%) ⬆️

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 63651d3...3ea9371. Read the comment docs.

@darthf1
Copy link

darthf1 commented May 14, 2019

What needs to happen to get this merged? Looking for this feature specifically.

@morphatic
Copy link
Author

In the meantime, you can install directly from my fork:

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

Hth

@ssuess
Copy link

ssuess commented May 21, 2019

Is there a plan to merge this soon? I also need this to be able to continue with vuex-persist. Thanks!

@championswimmer
Copy link
Owner

championswimmer commented May 22, 2019 via email

Copy link
Owner

@championswimmer championswimmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not contribute dist files to the repository. Those are autogenerated when I publish.

Also is it possible to add tests for the code you added, as that will prevent coverage % from falling.

@morphatic
Copy link
Author

Sorry. The dist files were not a part of the original PR. GitHub added them automatically later when I added them to my repo. I didn't know that would happen. I only added them to my repo because I wanted to be able to install the forked plugin directly from github rather than publishing the forked repo to npm. It's too confusing to figure out how to roll back some of the changes. I'm going to close this PR, delete my fork, create a new fork, then submit a new PR with just the necessary changes, adding tests. Look for a new PR shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants