RFC: plan for deprecating process hook's single-dispatch mode over the next couple versions

[jeffbski]

I wanted to see if anyone has any thoughts about this. I'm thinking it would be best to deprecate the process hook's single-dispatch mode which comes into play when you use the signature process({ getState, action }, dispatch)

// single dispatch mode enabled by including dispatch but not done
process({ getState, action }, dispatch) {
  dispatch(newAction) // call dispatch exactly once
}

We would keep the other two process hook signatures:

1. process({ getState, action }, dispatch, done) // multi-dispatch
2. process({ getState, action }) // return dispatching

We would also deprecate the dispatchMultiple option and add a warnTimeout option.

Here's some background on the issue and my proposed plan for deprecating is at the end.

tl;dr

The process hook has a variable signature that enables three modes currently (multi-dispatch, single-dispatch, and return dispatching). The original mode single-dispatch is confusing and easily misused by users.

Deprecating and eventually eliminating single-dispatch mode should reduce confusion and prevent many mistakes.

A new warnTimeout (configurable by option) will inform users in development if they are failing to call done after finishing dispatching. It can be disabled for intentional never ending logic.

Backstory

In that single-dispatch mode, the process hook expects dispatch to be called exactly once.

This is problematic since newcomers don't realize the nature of this, so they copy this single-dispatch code and will try to use it for multiple dispatches accidentally. It isn't obvious from the signature that they can't do this.

Another problem with single-dispatch mode is that since it requires dispatch to be called exactly once, you have to call it empty if you don't want to dispatch anything dispatch() which is also counter intuitive.

So while it seemed like a good idea originally for a common use case, it ends up being more confusing for people and that's not good.

However switching to the full signature (multiple-dispatch) form of process solves all of these problems with the slight disadvantage of an extra line of code to call done.

// normal multiple-dispatch mode
process({ getState, action }, dispatch, done) {
  dispatch(newAction) // call dispatch zero to many times
  done(); // call done when finished dispatching
}

To avoid confusion, I've changed the examples to use this since it is safer for people to learn from and they can do 0 to many dispatches as they see fit.

So going forward it is my proposal that we support only two of the three signatures for the process hook, dropping the single-dispatch mode.

1. process({ getState, action }, dispatch, done) // normal multi-dispach
2. process({ getState, action }) // return dispatching

For those that haven't seen the second form which was introduced with processOptions is great for working with promises, async/await, and observables. It uses the return value for dispatching:

• undefined - no dispatch
• object - assumed to be an action, so dispatched
• promise - dispatch the resolved/rejected actions
• observable - dispatch the next/error actions
• error - dispatch the error, wrapping if no type

You can also influence the dispatching with processOptions as mentioned in the docs.

Proposed plan for deprecating process hook's single-dispatch mode

deprecate single-dispatch mode - breaking change v0.12

1. add warnTimeout option w/default of 60s - (development ENV only) that warns in console.error if logic is still running after the elapsed time
   • logic is still running after 60s, forget to call done()? For non-ending logic, set warnTimeout: 0.
2. console.error in createLogic for process hook definitions that omit done and don't have either dispatchMultiple=true or warnTimeout=0 (!production ENV) that
   • single-dispatch mode is deprecated, call done when finished dispatching. For non-ending logic, set warnTimeout: 0
3. console.error in createLogic for processOptions.dispatchMultiple=true (!production ENV),
   • dispatchMultiple is always true in next version. For non-ending logic, set warnTimeout to 0

eliminate single-dispatch mode - breaking change v0.13

(We would delay v0.13 for at least a couple weeks to allow for consumers to get deprecation warnings in development)

1. dispatchMultiple will be true by default internally but no longer can be set as an option. Users that were using it for never ending logic will want to set warnTimeout: 0 instead
2. throw error in createLogic if process hook definition has dispatch but omits done and the warnTimeout is not set to zero.
   • done must be called when dispatching is finished or never ending logic should set warnTimeout: 0.
3. throw error in createLogic if dispatchMultiple is set (!production ENV)
   • dispatchMultiple option is no longer needed, set warnTimeout:0 for non-ending logic
4. retain the warnTimeout w/default of 60s as is (development) to help people from having unintentional non-completing logic.

[jktravis]

Hi, @jeffbski. This was definitely something that I overlooked when starting to use this tool. I'd probably spent a few hours on it until I finally read more closely and saw that I needed to include and call done.

With that said, I was converting from thunks where I was hastily coping and pasting my previous code into the process hooks.

Are you finding that it's new users who have used neither of the previous middleware that are overlooking this as well?

[jeffbski]

@jktravis Yeah, just recently a few people who just started with redux-logic ran into the same problem. They would take the single-dispatch code and just add a few dispatches to it, which would seem to be fine but then it doesn't work. So it just seems that single-dispatch mode is more trouble than it's worth. We want everyone to fall into the pit of success. They shouldn't have any of the problems if they start from the full dispatch/done multi-dispatch signature. And then the new warnTimeout will even warn them (console.error) if they forget to call done in development mode.

[KevinAst]

I personally like the variability provided through the arity of the process() hook.

This has become a common pattern used in such things as providing asynchrony to unit tests (i.e. the optional "done" parameter for asynchronous mocha tests).

Because I would assume that a single dispatch is more common, it seems inappropriate to require an additional done() call in the new proposal.

I personally think this is a training issue. If you should decide to keep the current API, any confusion could be mitigated in two areas:

1. More emphasize in the documentation.
2. The same development warnTimeout (proposed here) could be applied to the existing API as well:
   • single-dispatch mode: failure to call dispatch(), or incorrectly calling dispatch() a second time
   • multi-dispatch mode: failure to call done()

Thoughts?

[jeffbski]

@KevinAst That's an interesting idea that I hadn't considered. I'll ponder the failure cases for both situations some more.