ioof-holdings / redux-dynamic-reducer

Attach reducers to an existing Redux store
BSD 3-Clause "New" or "Revised" License
68 stars 9 forks source link

Recommended way of using withReducer() and react-redux's connect() #11

Closed anndroow closed 3 years ago

anndroow commented 6 years ago

Hi again,

I've been playing around with withReducer() and react-redux's connect(), and it seems to work fine as long as the reducer path provided in withReducer() is not a nested location. If it is, it'll result in TypeError: mapState must not return undefined..

To make it easier to describe this, I've done a CodeSandbox example app, see https://codesandbox.io/s/w0kp208jyw

If you navigate to "Test Route" you'll see the error message.

If you change

# /routes/Test.js

const TestModuleInstance1 = TestModule.createInstance("nested/testModuleInstance1");
const TestModuleInstance2 = TestModule.createInstance("nested/testModuleInstance2");

...to not have nested paths, it'll work.

I'm not sure if this is due to a bug or if I'm using react-redux's connect() incorrectly together with withReducer().

We really like your plugin and seeing it as an important piece in our application, so it would be awesome if it could work as expected.

Cheers!

mpeyper commented 6 years ago

Yep, something doesn't look right there.

it would be awesome if it could work as expected.

Things are always better when they work as expected.

You may be interested to know that we have a new library we plan to open source soon (I've been saying this for too long, I really MUST do it soon) called redux-dynostore that deprecates this one (it handles dynamic reducers as well as sagas and is way more extendible for other things in the redux ecosystem).

anndroow commented 6 years ago

Ok, then I know :)

@mpeyper oh it sounds awesome, can't wait for it. Do you think it'll be hard to migrate from redux-dynamic-reducer to that redux-dynostore?

Best regards

mpeyper commented 6 years ago

Migration should hopefully not be too bad. It's generally just a bit more verbose to allow more extension points.

We've opted for a store enhancer instead of a createStore replacement:

-import { createStore } from 'redux-dynamic-reducer'
+import { createStore } from 'redux'
+import dynostore, { dynamicReducers }  from 'redux-dynostore'

-const store = createStore(reducer)
+const store = createStore(reducer, dynostore(dynamicReducers()))

The use of redux-subspace is more explicit (and optional) when you wrap the component:

-import { withReducer } from 'react-redux-dynamic-reducer'
+import dynamic from 'react-redux-dynostore'
+import subspaced from 'react-redux-subspace-dynostore'
+import { attachReducer } from 'redux-subspace-dynostore'

-export default withReducer(myReducer, 'identifier')(MyComponent)
+export default dynamic('identifier', attachReducer(myReducer), subspaced())(MyComponent)

The only caveat I'm aware of at the moment that it doesnt support any of the options that you can currently pass to withReducer.

On top of this, a withReducer component is (currently) compatible with the dynostore(dynamicReducers() store, so you don't have to migrate everything all at once.

mpeyper commented 6 years ago

Hey @anndroow, redux-dynostore has been released. It still has this bug, but I'm working on fixing it over there instead of in this package.

anndroow commented 6 years ago

Hey @mpeyper, this is awesome! I'll take a look :) Yeah, makes sense to fix it in the new one instead.