Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for the Saga pattern by enabling multiple actions in a transaction #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vjancik
Copy link

@vjancik vjancik commented Jul 3, 2016

NOT FINISHED - DISCUSSION

I want to use this library in combination with Redux Saga. And with Redux Saga, you chain actions to create more complicated UI modifications. These chained actions can be synchronous or asynchronous.

And I want some of these actions to be reversible, and optimistic. To do that I need to be able to revert multiple actions that are part of the same transaction.

With my changes, this is possible. All actions with the same meta.optimistic.id get filtered out of the history on a revert.

This should be useful for all kinds of asynchronous action libraries where you can chain actions.
E.g. redux-saga, redux-thunk, redux-observable

And of course, all functionality before these changes stays unaffected.

@vjancik vjancik changed the title Add support for the Saga pattern by enabling multiple actions in a transaction. Add support for the Saga pattern by enabling multiple actions in a transaction Jul 3, 2016
@mattkrick
Copy link
Owner

Why not give each action its own id? Then just revert the action instead of the entire transaction. Assigning an id on a per-transaction basis makes for an all-or-nothing approach, right? If you have a test case in mind that'd probably help me understand the problem better.

@vjancik
Copy link
Author

vjancik commented Jul 3, 2016

Because then I have to keep track of all the IDs of all the actions in that particular transaction. And revert them all one by one.

Think of it like a DB transaction. You have a sequence of DB queries, in a transaction, and if any one of them should fail, all should fail. And you roll back the changes.

The equivalent of what you're proposing would be to roll back each of the queries on by one.

My particular use case is that I am composing my actions from smaller actions, to minimize code repetition inside the reducers.
So e.g. REQUEST_DATA, other than just fetching the data, also triggers SET_LOADING, and then CLEAR_LOADING when it finishes, then as a result of the data pull, I call another action action which loads the data that this action fetched.
Now if I want to optimistically set some variable before I confirm it with the server (with a subsequent REQUEST_DATA) I also have to rollback SET_LOADING, if it fails, or else my loading counter will be off.

I could make SET_LOADING reducer logic part of the REQUEST_DATA, but I use it in other places, so I'd prefer to compose them rather than duplicate them (the SET_LOADING reducer logic).

@mattkrick
Copy link
Owner

With that pattern, won't you have an entity integrity problem? As i understand it, you have a single loading boolean somewhere in your state that is turned true when something requests data & false when that data comes back. That means you can't make 2 requests for data. Alternatively, I'd suggest that loading is a computed property. Your loading flag is true when getState().history.size > 0.

If you have multiple loading flags in your state, then each will be managed by its own reducer & this isn't a problem.

PS, if you're doing async work with your own server, I'd suggest you try cashay, which is like relay, but for redux: https://github.com/mattkrick/cashay. it makes life MUCH easier.

@vjancik
Copy link
Author

vjancik commented Jul 3, 2016

No I have a loading counter, 0 <> +Inf, and a global Loading indicator, which is indicating something is loading if the counter is positive, then I have a per-entity loading boolean, to indicate if that particular entity is currently being loaded.

Thank you for the suggestions! I'll check out cashay.

So what do you think of these changes? I think they are a nice inexpensive improvement to the current functionality.

@mattkrick
Copy link
Owner

yeah, it looks minimally invasive & i don't see a huge performance hit to include the changes. I'm not sure I understand it 100%, but if you write up a couple unit tests, I can dig in a little more. In the meantime I'll get a TravisCI on this repo

@clayne11
Copy link
Contributor

This sounds awesome. I would love to have multiple actions in a transaction.

@vjancik
Copy link
Author

vjancik commented Nov 18, 2016

@clayne11 It's not that much code. If you'd like to take over, please go ahead! Right now I unfortunately don't have time to finish this work :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants