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

It doesn't work when Nested Persists combine with Hot Module Replacement #940

Closed
nishino-tsukasa opened this issue Dec 9, 2018 · 4 comments

Comments

@nishino-tsukasa
Copy link

nishino-tsukasa commented Dec 9, 2018

Hi, @rt2zz
It doesn't work when Nested Persists combine with Hot Module Replacement.
I implement Nested Persists in my project. It works very well。
Recently, I need to use HMR, but I found only work first persist in Nested Persists.
Example

const rootPersistConfig = {
  key: 'root',
  blacklist: ['A']
}
const APersistConfig = {
  key: 'A'
}
const rootReducer = combineReducers({
  A: persistReducer(APersistConfig, AReducer),
  other: otherReducer,
})
persistReducer(rootPersistConfig, rootReducer)
...
if (module.hot) {
    module.hot.accept(() => {
      store.replaceReducer(
        persistReducer(rootPersistConfig, nextRootReducer)
      )
      store.persistor.persist()
    })
  }

I call persist() after calling store.replaceReducer. Other state be persisted after updated, but A state not be persisted after updated.
I found out by reading the source code that in the 108th line in the persistReducer.js file.

if (_persist) return state

The action is not propagated down, causing _paused in other persisters to be equal to true and not becoming false. I forked your project and changed the 108 line of code to

if (_persist)
  return {
        ...baseReducer(restState, action),
        _persist: { version, rehydrated: false },
  }

, and it worked. I am not sure if this change will cause other bugs. I want to know if there is a problem with the usage or a bug in the project itself.

@aguynamedben aguynamedben changed the title It doesn't work when Nested Persists combine with Hot Module Replacement (V5) It doesn't work when Nested Persists combine with Hot Module Replacement Dec 11, 2018
@aguynamedben
Copy link
Collaborator

I am not sure if this change will cause other bugs. I want to know if there is a problem with the usage or a bug in the project itself.

I'm not sure either. @rt2zz might be able to help when he has time.

@DavideDaniel
Copy link

Can confirm that we are seeing this as well. I was looking for information about why only the first persistoid was working, and came across this. The last change that went in along with multiple nested reducers was us fixing hot module replacement...

@sandinosaso
Copy link

Can confirm this. I am seeing this exact behavior using 5.10.0

@jscinoz
Copy link
Contributor

jscinoz commented Jun 6, 2019

Can confirm this same issue, albeit in a slightly different use-case: we use aggressive code splitting and dynamically attach & detach reducers as the user navigates to different views in our application. Some views have view-specific reducer state that needs to be persisted, so we use a nested setup similar to @nishino-tsukasa's example.

Even though persistor.persist() is always called after we swap out the reducers (store.replaceReducer() (related: #850)), this only resumes persistence at the top level; nested reducers never see the PERSIST action as it isn't propagated downwards due to this particular part of persistReducer:

if (_persist) return state

As a result, the nested reducers remain in the paused state indefinitely and no state for them is persisted.

jscinoz added a commit to AvokaTech/redux-persist that referenced this issue Jun 6, 2019
jscinoz added a commit to AvokaTech/redux-persist that referenced this issue Jun 6, 2019
@jscinoz jscinoz mentioned this issue Jun 6, 2019
@rt2zz rt2zz closed this as completed in 5ad36b8 Jun 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants