react-redux-modal-provider
react-redux-modal-provider copied to clipboard
[RFC] Modal provider improvements
Alright, just want to start a discussion about what concerns me a bit about this project and how IMHO we can make things better)
- First of all, the main architectural idea of keeping react components in redux state seems fundamentally wrong. Because redux state should be completely serializable so that server side state hydration and time travel are possible. An alternative solution would be to just pass modal components directly to ModalProvider, like this
import modalsConstants from './modals.constants.js';
import LoginModal from './login.modal.js';
import RestorePasswordModal from './restorePassword.modal.js';
import ModalProvider from 'react-redux-modal-provider';
const modals = {
[modalsConstants.LOGIN_MODAL]: LoginModal,
[modalsConstants.RESTORE_PASSWORD_MODAL]: RestorePasswordModal,
}
<ModalProvider modals={modals}/>
- Then, passing components to showModal action also looks not quite right. Because often you want to call your modal actions from within other actions, using redux-thunk or other solution for handling side-effects. And you really don't want to require your components in the actions files, because you don't want to mix universal/business-logic code with ui-related code. Passing modals to ModalProvider can also solve this issue. So invoking showModal action will look like this
import modalsConstants from './modals.constants.js';
showModal(modalsConstants.LOGIN_MODAL, {/** props */});
-
Using eventemitter to avoid wrapping components in connect also doesn't look like a good idea for several reasons. By doing it you introduce extra uneeded dependency, add one more abstraction layer and a bit of magic, that could be confusing for the users. There is nothing wrong with wrapping component in connect. It's a standard way of doing things in react/redux stack and users have already got used to do this. Explicit is better than implicit. There won't be any performance implications because of extra connect wrappers, considering that you only provide mapDispatchToProps. For example react-router guys have recently removed subscription system responsible for handling context updates from react-router 4. Because they came up to conclusion, that it was confusing, complicated and not really react-way-ish)
-
Having two different providers also is a kinda strange solution. What if you want both behaviours? What if you want to switch this stacking/non-stacking behaviours in runtime? How would you do that, using two providers? I think it would be better to handle it using different actions, like showModal and showSingleModal (though I don't know yet how to implement it).
So these are my main concerns and suggestions. Please, tell what do you think about it and what is your personal vision)