salsita / prism

React / Redux action composition made simple http://salsita.github.io/prism/
496 stars 24 forks source link

saga not canceled when removing item #54

Closed namjul closed 7 years ago

namjul commented 7 years ago

I tested it with the dynamic-list-of-counters example. Updating it to redux-elm 3 and adding a saga to the counter i get the following situation. Adding a counter after i already added and removed one, it throws the following error:

'The Saga instance has already been mounted, this basically mean that your Updaters do not form a tree, please be sure that every Updater is wrapped by another Updater (except root updater). It does not make sense to use combineReducers for Updaters.'

During the removal it does not seam to cancel the saga, so that it is still in the SagaRepository.

tomkis commented 7 years ago

This is actually much broader problem.

The issue is that when you remove the model from appstate which triggers re-render and this re-render calls componentWillUnmount which dispatches Unmount. However, when Unmount is being processed again, the model does not exist in the app state anymore, therefore it can't be passed to corresponding sub-updater which would handle the action.

The logic indeed makes sense, but it also means that we shouldn't allow user to react to Unmount action and saga unmounting should happen in the root level. See the commit.

This will introduce a breaking change, since Unmount action will be removed from the exposed API.

namjul commented 7 years ago

Can you explain why is it that we have to disallow the user to react to Unmount or is it technically not possible?

namjul commented 7 years ago

@tomkis1 There is still an issue with concatenating action patterns. For example i get the following action when using the ParameterizedMatcher which never ends the id with a dot. Explore.QueryList.Query.0DimensionFilter This causes a missmatch at moun/unmount in the sagaRepository.

tomkis commented 7 years ago

Fractability of Sagas now lives in user-land. Closing.