Add flattenCmd
#263
Add flattenCmd
#263
Conversation
src/cmd.js
Outdated
| ) { | ||
| return flattenCmd(cmd.nestedCmd); | ||
| } | ||
| if (cmd.type === cmdTypes.LIST) { |
| return cmd.cmds.flatMap(flattenCmd); | ||
| } | ||
| return [cmd]; | ||
| } |
There was a problem hiding this comment.
Is there any use case to justify including Cmd.none values in the array? It might be more useful if the array consists only of run commands and action commands.
There was a problem hiding this comment.
I can't really think of a use case other than a testing purpose which would still be covered by checking expect(flatCmd).toEqual([]). I've updated to remove Cmd.none values 👍
There was a problem hiding this comment.
I'm torn on this. I kind of like keeping the original cmds in there. though i guess there's not much value. Why filter them out though? The general idea with a flattened list is to say it contains a command right? if someone wants to look for Cmd.none, we should not stop them
index.d.ts
Outdated
| @@ -266,3 +266,5 @@ export function isLoop(test: any): boolean; | |||
| export function getModel<S>(loop: S | Loop<S>): S; | |||
|
|
|||
| export function getCmd(a: any): CmdType | null; | |||
|
|
|||
| export function flattenCmd(cmd: CmdType): CmdType[]; | |||
There was a problem hiding this comment.
The return type could be more specific:
ActionCmd<UnknownAction> | NoneCmd | RunCmd<UnknownAction, UnknownAction>
| @@ -4,6 +4,10 @@ export const flatten = (array) => { | |||
| return concat.apply([], array); | |||
| }; | |||
|
|
|||
| export const flatMap = (array, mapFn) => { | |||
| return array.reduce((nextArray, cur) => nextArray.concat(mapFn(cur)), []); | |||
There was a problem hiding this comment.
Since there are instructions in the docs on how to use this library with browsers that don't natively have Promise or Symbol I felt I should avoid using Array.prototype.flatMap as none of those browsers would support it either.
|
Thanks for the updates @abradley2! |
index.d.ts
Outdated
| @@ -266,3 +266,5 @@ export function isLoop(test: any): boolean; | |||
| export function getModel<S>(loop: S | Loop<S>): S; | |||
|
|
|||
| export function getCmd(a: any): CmdType | null; | |||
|
|
|||
| export function flattenCmd(cmd: CmdType): RunCmd<UnknownAction, UnknownAction> | ActionCmd<UnknownAction>; | |||
There was a problem hiding this comment.
you accidentally got rid of the array portion of the return type here.
src/cmd.js
Outdated
| cmd.type === cmdTypes.SET_TIMEOUT || | ||
| cmd.type === cmdTypes.SET_INTERVAL | ||
| ) { | ||
| return flattenCmd(cmd.nestedCmd); |
There was a problem hiding this comment.
we lose some info here. A map cmd is more than just the nested Cmd. it's also the tagger function. Is there any way we can preserve that in this?
There was a problem hiding this comment.
For our tests we haven't ever really felt the need to consider the tagger, but I do see how that can be a relevant detail. I also suppose even Cmd.timeout and Cmd.interval could be.
docs/tutorial/Testing.md
Outdated
| import { Cmd, loop, flattenCmd, getCmd } from 'redux-loop'; | ||
|
|
||
| test('reducer fetches data after loadingStart action', (t) => { | ||
| t.plan(2) |
docs/tutorial/Testing.md
Outdated
| You may need to test objects from the `Cmd` module that make use of | ||
| `Cmd.list` or `Cmd.map`. Imagine in the above example the reducer is updated to | ||
| perform multiple commands when loading starts. We want to test that the properly | ||
| shaped `fetchDetails` cmd is called, but don't necessarily want to mock out every |
There was a problem hiding this comment.
what do you mean mock out effects? You shouldn't need to mock any effect because they are not run
There was a problem hiding this comment.
updated this language to be a bit more clear 👍
docs/tutorial/Testing.md
Outdated
| reducer(state, loadingStart(1)); | ||
| ); | ||
|
|
||
| const fetchCmd = flattenCmd(resultCmd).find((cmd) => { |
There was a problem hiding this comment.
it seems a little odd to search by successActionCreator only to then do a deep equal later. Why not just say
expect(flattenCmd(resultCmd)).toContain(
Cmd.run(fetchDetails, {
successActionCreator: loadingSuccess,
failActionCreator: loadingError,
args: [1]
})
)There was a problem hiding this comment.
This and t.plan where tape testing library specific. I've updated the examples to use the "jest" assertions which make the example tests a bit more clear
Motivated by the discussion here, we have an application making extensive use of
redux-loop'sCmd.mapandCmd.listfunctionality to support sub-modules (very similar to how Elm apps are traditionally structured) and want to test commands in a convenient way.Example fork used is published here. In our tests we had a dozen situations where we used a similar
getRunCmdfunction that did much of the logic inflattenCmdand then did a.findImplementing that function now looks something like: