logux / redux

Redux compatible API for Logux
https://logux.org/
MIT License
127 stars 16 forks source link

Support deferring logux config until client start #4

Closed hardchiu closed 6 years ago

hardchiu commented 7 years ago

First of all, thanks for the great lib.

Reason

Recently I try to use logux on a project that need authentication, but found the auth workflow provided by logux not enough for do it properly.

Since the current version of logux redux require to config the client when create the redux store, it require us to provide credential and userId when redux store is created.

Normally we do the login screen with redux store available, so we need to defer the config of client until it is started. Another option will be letting the logux client start anonymously and do the auth within logux protocol, but this way is far harder to patch (since the log store name is created with userId in config), so I patch it with defer config for my quick use.

Config when client start

I have added two methods to start the client

  1. By the enhanced store startLoguxClient(config) method
  2. By dispatch action { type: 'logux/start', config: {} }

The action is added since we are starting the client in application logic (i.e. after login, in our case a saga to be exact), which at that place the client/store are hardly accessible.

Skip log before start

Before the client is started, there maybe some subscribed react component will add 'logux/subscribe' to log. I have surpassed any log before start. Free feel to comment if that is not desired. I have also added a reducer to report the started state and patched react-subscribe to only subscribe when logux is started, see https://github.com/logux/react-logux/pull/2.

Redux enhancer

In this PR, I changed to do the job with redux enhancer, which can be consumed by:

createStore(rootReducer, rootState, loguxStoreEnhancer)
or
createStore(rootReducer, rootState, compose(...[loguxStoreEnhancer, otherEnhancer])) 

In this way, we can keep the createStore chain, and so that we can use logux together with tools like reactotron https://github.com/infinitered/reactotron/blob/master/docs/plugin-redux.md

Backward Compatible

To make it backward compatible, I tried to create the createLoguxCreator function with loguxStoreEnhancer. The logic should be the same as before.

ai commented 7 years ago

Good question. I will need few days to think about it, since I think that this way maybe not the best one (but the problem, of course, should be fixed).

Is it OK?

ai commented 7 years ago

I took time because this question is not about the delay between logux was started and a user was authenticated. What if we will need to change user during the Logux runtime and save user-independent state?

ai commented 7 years ago

Hm, maybe store.client.changeUser(userId, credentials) is a better idea? What do you think?

hardchiu commented 7 years ago

My patch is basically work with the current logux auth design. If the underlying auth handling can be improved to handle per user state, it should be more flexible. It can enable implementation to do authentication with logux.

The underlying log store handling for changeUser is likely very complicated (to me). Before the changeUser method is ready, I will use my patch as temp solution, as it should be good for me to use.

As my original comment mentioned, it will be hard to get the store / client in the application logic, maybe it is better to also provide changeUser with redux action.

Beside that, seems a logout method is need for handling offline / stored log (or it sounds like logout is just shorthand to changeUser(false)).

ai commented 7 years ago

changeUser was just brainstorm :).

It is awesome that you understand this big and complex project and make so big PR.

You can use your fork by "logux-redux": "hardchiu/logux-redux" right now. Without waiting for this PR.

Sorry, that I don’t accept PR right now. I understand the problem. But my developer intuition asks me to think a little before accepting big API changes. What if developer will execute dispatch.sync() before logux/start?

ai commented 7 years ago

I found today other big of Logux Redux which can be related to this fix. Actions could have class instances. But we can store only very simple objects in IndexedDB.

Of course, we can check action content for dispatch.sync and dispatch.crossTab, but local dispatch must work with any legacy code.

Seems like we need to store tab-specific actions in the memory.

In this case user changing could be easy. We will change IndexedDB, but local actions from memory will be the same.

Sorry, this issue could take few weeks (mostly because I need a rest after ReactiveConf). Hope it is OK to you to use fork.

hardchiu commented 7 years ago

If the dispatch.sync is not intended before logux/start, it is better to throw an error. If you mean some intended dispatch.sync, user can start an anonymous client to do that, which can be done with both implementation.

I can use my fork at the moment. It would be better to have more time for you to think it more thoughtfully.

For the crossTab things, it is really complicated. I have a more blocking issue there, maybe I can start an issue with details in logux-client.

In short, I found the crossTab state seems not being synced for action dispatched from server in subscribe (i.e. channel > process). The leader got the action but other subscriber tab will not sync the state. It make the non-leader tab cannot be initialized for the subscribe.