Cmd.setTimeout, Cmd.setInterval

[laszlopandy]

Add a new type of action which schedules another command to be dispatched using setTimeout() or setInterval(). This is a very useful feature for starting/stopping animations and scheduling timeouts.

Having an explicit action type makes the intention of the code clear -- you don't need to use Cmd.run() for something which is not an external side-effect.

[bdwain]

I like the thought behind this, but I'd like to see it be more generic. I think it would be good to build in support for intervals as well as timeouts, and also to support cancellation. Without those, I wouldn't feel comfortable adding it to the API because a timeout that can't be cancelled is going to shoot a lot of people in the foot (and you can do this pretty easily with Cmd.run).

I also think it would be good to see if we could just make all effects delayable, rather than make individual ones for action.

Here's an API idea I had.

Cmd.delay(otherCmd, { delay: 1000, interval: true, onScheduledActionCreator: myActionCreator })

and then you could use the scheduled action creator to store the timeout id in case you wanted to cancel it later. And the interval option (open to other naming suggestions), would control whether it was a timeout or interval.

Thoughts?

[laszlopandy]

Great ideas to make this more general. Scheduling intervals is something we have in our code, and it always feels more awkward that it should be.

My original thought was that you can use Cmd.delay() to do intervals by returning another Cmd.delay() after each action is processed. Similarly, there is no need to cancel because you can just set a flag in your state that the next action should be ignored. While this works in theory it isn't very natural especially for programmers who are used to using setTimeout() and setInterval(). Which begs the question: should we just use those names (even though I think delay is much better) because of familiarity?

Cmd.setTimeout(otherCmd, 1000, Actions.setTimeoutHandle)
Cmd.setInterval(otherCmd, 1000, Actions.setIntervalHandle)

Alternatively I would pick a similar name, but keep the APIs separate:
Cmd.delay(otherCmd, 1000, Actions.setDelayHandle)
Cmd.repeatingDelay(otherCmd, 1000, Actions.setDelayHandle)
Or
Cmd.schedule(otherCmd, 1000, Actions.setDelayHandle)
Cmd.scheduleRepeating(otherCmd, 1000, Actions.setDelayHandle)

[laszlopandy]

@bdwain I implemented Cmd.schedule and Cmd.scheduleRepeating, and updated the tests. Let me know what you think, and if it's good I'll update the docs too.

Edit: Oops, I forgot the cancel method too

[bdwain]

Sorry for the delay in looking at this. With everything going on in the world I lost track of things.

I think I'm torn between the name delay and setTimeout/setInterval. But i definitely like them both more than schedule. I'm also torn between having 2 separate Cmd types or one with a boolean option to switch between the two. I feel like there's cognitive overhead when you add extra Cmd types. Parallel and non-parallel lists used to be two different command types, but then we combined them into one and I think it worked better (See discussion here)

I think we should either do Cmd.delay with a boolean switch option or Cmd.setTimeout and Cmd.setInterval as separate ones (sharing most of their implementation) because setTimeout and setInterval aren't new concepts to people so there should be little cognitive overheard, but delay would. I'm pretty torn though between the two though. Thoughts? I think I'm leaning towards separate Cmds, just because it's consistent with web APIs so it'd be pretty straightforward.

Other than that, the API seems good to me, except I'd like to have the third parameter be an options object. I've had that come back to bite me too many times. We'll want to add some option in the future but having a never ending parameter list is hard to use. Also, while otherCmd and the number of milliseconds is required, the action creator is not.

I'll comment on implementation details, but I like the direction this is heading.

[bdwain]

I polled everyone I work with and they all prefer setTimeout/setInterval also. I think that is a good way to name it

[laszlopandy]

Hey @bdwain. I'm also sorry for the delay. I lost some github notifications and didn't check up on this.

I also agree that Cmd.setTimeout and Cmd.setInterval is preferable because it matches the existing functions and everyone knows what they mean. On the other hand that means we should inherit the cancel names too.

Are you okay with these names?

• Create methods: Cmd.setTimeout() and Cmd.setInterval()
• Cancel methods: Cmd.clearTimeout() and Cmd.clearInterval()
• Types in Typescript and flow: SetTimeoutCmd<A> and SetIntervalCmd<A> with type: "SET_TIMEOUT" and type: "SET_INTERVAL" respectively.

The other option is to have a single type DelayedCmd<A> (type: "DELAYED") with an isRepeating attribute and make it clear that Cmd.setTimeout() and Cmd.setInterval() are just two different constructors for the same kind of command. WDYT?

Edit: I've decided it's best to go with one unified command type ("DELAY"). The implementation is done unless we find any other big issues.

[bdwain]

ah yea I guess we do want clearTimeout and clearInterval cmds also. As far as separate or shared types, I'll defer to you on that one since i haven't done a ton of typescript yet. Though I'm curious what @nweber-gh thinks.

[bdwain]

sorry didn't mean to close this.

the code looks good. Besides my few comments, and the conflicts that got introduced from #235, I think this is probably good to go

[bdwain]

also, if you end up merging before I look at this again, please remember to squash before you merge

[bdwain]

also, don't forget to use the wrappedDispatch function as discussed in https://github.com/redux-loop/redux-loop/pull/235/files#r397694600

[nweber-gh]

Functionally I think that the single delay interface is probably fine.. however, I feel a bit weird about the TypeScript definition not matching the public API. The other main Cmd generators returns a type that matches the function and it feels like it’ll add a bit of cognitive delay to have to process what a DelayCmd is instead of just seeing a SetTimeoutCmd.

But.. I also don’t have a personal problem with the abstraction. It makes sense, it just adds an extra mental jump that folks have to make when tracing through the types.

[bdwain]

@laszlopandy do you think you'll get back to this soon? I'd love to release a new version soon

[laszlopandy]

@bdwain Yep. I was busy with family stuff recently but I'm been thinking about how to finish this up. I'll set aside some time this week.

[bdwain]

sounds good thanks @laszlopandy

[laszlopandy]

TODO:

• Fix conflicts
• Remove flow definition changes
• Use two separate interfaces in TypeScript definitions
• Use wrappedDispatch
• Add tests for Cmd.setInterval
• Verify that TypeError: Illegal invocation is fixed by Don't let this point to cmd object if user forget to use an arrow function #242

[nweber-gh]

Do we support passing arguments, similar to RunCmd, for the delayed commands? Both setTimeout and setInterval support an arbitrary set of args, which get sent to the callback function, so I think it would make sense to support args in these commands as well.

example

setTimeout((message, count) => console.log(message, count), 500, 'hello world', 7);

[nweber-gh]

Or.. Perhaps that doesn't make sense, since this is delaying other commands. We could just delay a RunCmd to get args. It might make sense to support args to pass to the scheduleActionCreator however. What do you guys think?

[bdwain]

Yea I think the callback args aren't really necessary. Since this is taking other Cmds, we can just pass those args directly to the cmd if needed. And for passing args to the action creator, we can just use arrow functions like we've done with Cmd.run.

const runCmd = Cmd.run(
  (message, count) => console.log(message, count),
  {args: ['hello world', 500], successActionCreator: whatever}
)
return loop(newState, Cmd.setTimeout(runCmd, 1234, {scheduledActionCreator: id => actionCreator(id, arg, arg2)}))

[laszlopandy]

@bdwain This PR is done, feel free to merge it whenever you are ready.

[bdwain]

shouldn't we mock the timers and wait for those rather than just resolve as soon as dispatch is called? That would allow us to verify we're actually waiting for the right amount of time, and would allow us to improve the setInterval test by verifying it actually runs multiple times

[laszlopandy]

Yes, do you have a nice way to do this, or would you just overwrite the global setInterval and restore it in an after block.

[bdwain]

jest can do this https://jestjs.io/docs/en/timer-mocks

[laszlopandy]

Done, please check it again.

[bdwain]

can you do the same thing for setTimeout? basically the same as the setInterval test, except we verify that we don't dispatch more actions after the time elapses again

[laszlopandy]

Done.

[bdwain]

thanks @laszlopandy !