feat: optional commit/rollback

[sebasgarcep]

Makes commit/rollback actions optional.

Could anyone review this commit? @wacii @sorodrigo @bsouthga

[sebasgarcep]

I'm noticing this solution won't be enough. Every one of those complete actions dequeues from the outbox. I don't understand why dequeueing is coupled to the commit or rollback behaviour. What if you don't want anything to happen when the update succeeds (For example, when you know an action will eventually succeed and you don't care when it does).

[wacii]

You could specify a default action in complete(). There needs to be some event for the outbox to get updated, even if the user doesn't specify one.

Why did you change what send() returns? The value it resolves to isn't used anywhere and what was chosen isn't particularly meaningful, so I suppose it doesn't really matter.

[sebasgarcep]

I changed what send() returns because eslint was complaining about incosistent returns with the changes. I think what send() returns is never exposed to the end-user so it doesn't really matter. Please correct me if I'm wrong.

[sebasgarcep]

About the default complete action, I'm thinking that if the user doesn't provide a commit mechanism, then it should complete with an action of type Offline/COMPLETE_EFFECT and the result of the effect as the payload. Is that fine?

[wacii]

That sounds good.

[sorodrigo]

I agree with your suggestion. What about naming it Offline/COMMIT_EFFECT or Offline/DEFAULT_COMMIT?

[sebasgarcep]

I've added the default commit action and added a test. Now that I think about it, do we also need a default rollback behavior?

[sorodrigo]

I think rollback is also a nice feature to have. I would add the request action type as a meta inside the defaults, because if you start abusing this feature you won't be able to trace the request that resulted in the commit/rollback. Other than that seems good!

[sorodrigo]

@sebasgarcep could you implement the last changes so we can merge this?

[sebasgarcep]

@sorodrigo I've had a very busy couple of days, but I now have some time to work on it. :)

[sebasgarcep]

@sorodrigo @wacii I've merged develop into this branch. I think this pull request is ready, and it's waiting for you to merge into develop.

[wacii]

@sebasgarcep You handle default commit and default rollback dispatches differently. If the user specifies config = { ...defaultConfig, defaultRollback: null }, then rollback actions will not be dispatched and failed offline actions will never be removed from the queue.

[sebasgarcep]

@wacii If the user provides config = { ...defaultConfig, defaultRollback: null }, even if we dispatch the action it will fail as redux only accepts actions where type is a non-empty string. I think there are two solutions to avoid runtime errors: either we throw during initialization if defaultRollback or defaultCommit are not proper redux actions (what I would do), or we silently replace those objects with our defaults.

[wacii]

@sebasgarcep That's true, but I think the resulting error message from redux would be enough to direct the user to a solution. With if (rollbackAction) dispatch(complete(/* ... */)), the failure is silent. Redux Offline just stops working without any feedback to the user.

I would handle the rollback actions the same way you handled the commit action. Just pass the rollbackAction to complete() without checking if it exists or not.

We could throw or warn if the user provides an incorrect defaultRollback or defaultCommit during initialization as well. But I think this should be in addition to the error handling provided by Redux, not instead of.

[wacii]

Alternatively you could remove defaultRollback and defaultCommit from the config. A little less flexible, but it would make the code simpler and more reliable.

[sebasgarcep]

@wacii I don't think we should remove defaultCommit or defaultRollback, @sorodrigo presented some nice reasons why. I'm removing the ifs from the rollback statements and warning the user if their defaultCommit or defaultRollback is incorrectly typed.

[wacii]

@sebasgarcep I was suggesting keeping defaultCommit and defaultRollback, but not allowing the user to change them in the config. But I'm fine with it as is.

[sebasgarcep]

@wacii Sorry I misunderstood. I've made the changes. Could you review them?

[wacii]

@sebasgarcep Changes look good. Ultimately I'd like to use Flow to provide type checking for config.defaultCommit and config.defaultRollback, but that's a mess right now.

Added a couple small changes. Extended tests to look for meta.completed, something that should have been tested before, as that is how the reducer knows to enqueue and dequeue.

Also added the offline action as metadata onto the default actions.

[wacii]

Flow fails... I'm just going to revert my changes for now.

Alright, I'll revisit my changes later today.

[wacii]

It was a randomly failing test, which I'll have to look into, but it passes for now.

Adding the offline action into meta is the only meaningful change. Do we just want that on default actions or all of them?

[sorodrigo]

Do we just want that on default actions or all of them?

I think only on default ones because those are anonymous actions towards redux devtools. Thanks a lot @sebasgarcep and @wacii 🎉!

[sebasgarcep]

Thanks @wacii for your changes!

[sebasgarcep]

Thanks @wacii @sorodrigo for making this possible! Could we also open an issue with a roadmap before the next release?