react-contextual
react-contextual copied to clipboard
External Stores stop being external as soon as you provide them
It looks like the actions in a store get bound to the Provider when it renders. This means if you unmount a provider and later provide the same store, the actions will throw an error.
I mocked up what was happening in my project here: https://jp46nlpk8y.codesandbox.io/ https://codesandbox.io/s/jp46nlpk8y?view=preview
@d3dc That's super odd, should happen in the constructor - but perhaps it tried to bind functions that are already bound, causing some distress. I'll investige ...
I may be conflating two problems. I was getting this react warning in my own code:
warning.js:33 Warning: Can't call setState (or forceUpdate) on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.
~~This was also working last week in v4~~
I'm wrong, this only worked when I did:
<Provider store={{ ...DrawerStore, initialState: { open }}}>
...
</Provider>
which is a bit different since the store reference changes. (actions is a shallow copy tho...)
Like you said, it looks like this happens in the constructor: https://github.com/drcmda/react-contextual/blob/master/src/store.js#L65
The Provider specific actions are Object.assigned to the store state. So they are persisted.
AFAIK, this is unwanted? I'm not sure why this exists here. Is it safe to just remove it?
I might be confused as to the purpose of the external store. It seems like the use case for this is to be able to manipulate the provided state from outside of the Provider through the store reference.
External stores make it easier to maintain multiple stores throughout the app and they can be accessed and mutated from outside react, in the rare instances this is needed. I think i would normally create one store instance and never, ever render it twice - but for sure this is still a problem if if doesn't support that.
@d3dc Can you try with 5.0.2? It should work now ...
This still puts Provider bound actions in the external store, they just will not stack (see the example with both Containers shown).
I'm working on a PR that does a bit more heavy-handed stuff. I introduced a utility that maps the actions from getStateUpdateFunctions to wrapper functions. createStore then wraps it's state update functions with a new explicit setState. Provider's can then rewrap and put bound versions of those functions into their internal state.
I'll try to get it up before I stop for the night. Of course, I realize, this is still not right, as the store would then not need to be Provided
As far as motivation, i don't want to always provide the store, as I may never need it unless a page loads it. The trigger for needing to load it is having a Container. Wouldn't it make sense for Container to automatically provide it? And then some pages have two Containers (that should share state).
I've got some "pseudo" code over on my fork. Compare
This is very different from what was initially there. Providers "shadow" the external store - only consumers that exist inside the context can directly affect their state and they can even have different initialState.
The problem is you need to sync up external actions to Provider internal state. Would it help if Providers were subscribing to the state deltas instead? Thats some added complexity for this.
For some reason my mind latched onto being able to stack middleware onto the store, but I think that is another custodial closet.
@d3dc interesting ... i've never though of stacking actions/re-using stores, to me i thought of them singletons. As long as the other cases don't break, sure, push your PR and let's figure it out.