Not sure about general idea of this command but I feel that implementation is little bit naive as it does not update state in a sequence. I believe that snippet below does the job.
export type EditorCommand = (state: EditorState, dispatch?: (tr: Transaction) => void) => boolean;
export function chainCommands(...commands: EditorCommand[]): EditorCommand {
return (state, dispatch) => {
const dispatcher = (tr: Transaction) => state = state.apply(tr);
const last = commands.pop();
const reduced = commands.reduce((result, command) => result || command(state, dispatcher), false);
return reduced || last !== undefined && last(state, dispatch);
};
}
If this is wrong or unwelcome then I’d like to ask for a rationale for current behaviour that I could missed. Thanks!
The function works as intended—it applies the first command in the sequence that is applicable (returns true), and should never accumulate the effects from multiple commands.
I felt little bit misguided by the comment inside describing the function, i.e. Combine a number of command functions into a single function (which calls them one by one until one returns true).
Maybe this comment (and the function name) could be updated to be little bit less misleading (at least in my opinion!)
Maybe it’s just me but I feel this is quite different than
it applies the first command in the sequence that is applicable (returns true), and should never accumulate the effects from multiple commands
which clear and straight-forward to me. I’d see just this text in the docs as more descriptive (note that current description does not explicitly say about the state changes, just calls).
“Combine commands” has a “combine commands’ results” flavour in it (in now that’s not was written yet it caught me off-guard). On the other hand “chain” brought to my mind a builder (and fluent interfaces) which also preserves the state.
I’d like to propose another name for it but really nothing comes to my mind—maybe “selectCommand” or “chooseCommand” would give proper intuitions…?
I do feel “combine” is confusing here. If you don’t read carefully, the impression would indeed be that chainCommands applies all of the commands, not just the first one that returns true.
