I’m happy for commands to stay as they are, but what I’d propose is decomposing the implementation into “mutators” (open to a better name) that are composable. The end result should be commands that maintain their current interface, but whose internals are probably just a couple of lines combining mutators and calling dispatch.
I think I’m missing something regarding the difference between transactions and editor state, specifically regarding what state they encapsulate. What’s missing from transactions that prevent commands from only working with transactions? It’s surprising to me that it’s not an explicit goal for commands to work on transactions, so I believe there’s some consideration that I’m overlooking.
I suspect we made a mistake with this thread in conflating transforms with commands — commands can continue to live as-is, but (I think what @wes-r and) I want is something new that’s similar to commands, but is a lower-level composable construct. In practice I’d probably end up never using commands, and always build my own because of specific user-experience requirements that I’m aiming for.
Having two functions, one to check whether something applies and one to execute it, is how things initially worked but it was really hard to keep those two consistent, so I moved to the one-function-with-optional-dispatch form, and it made the code much simpler and less error prone.
Adding another concept of helpers that implement the functionality of the commands is possible, but I’m not really convinced the added value is worth the extra API surface.
FWIW I came across this thread as I was running into the exact same problem - I’ve been using the built in commands, and they’re great up until I need something slightly different, or I need to compose a few of them together, but ultimately I’m having to just copy the internals of them out of prosemirror-commands and reimplement locally.
@bradleyayers idea of a lower level composable interface would be ideal for me, and I think I’m in a similar boat - I doubt I’d end up actually using any of the built in commands, as nearly all the behaviours I’m after are just slightly different to the built in commands.
For me, I think it’s worthwhile to have the extra API surface - IMO I don’t think it would be too much cognitive overhead for people coming to ProseMirror, and it would be incredibly worthwhile. I found it really confusing that I couldn’t chain together commands (until I properly understood their inner workings) - indeed that’s what I thought the chainCommands helper initially did. Having a different set of ‘utilities’ would help clear this up I think.
Ping @marijn - I hit this again! The liftListItem implementation in prosemirror-schema-list is awesome as it will do the right thing for any nested list items. The issue I ran into is wanting to invoke it several times to lift an item ALL THE WAY out of the list (a “remove” command). Presently there is no good way to do this. I had to copy the implementation and modify it to operate on a transaction so that I can invoke it in a loop.
All of this great functionality is locked away behind dispatched commands. Instead they should be building blocks! Seems many of us long term users have solved this by copying the innards of the commands we want to build upon. This is not good! It makes managing our code harder and it adds a barrier to entry for beginners as they will inevitably run into the same desire and the same less-than-perfect-solution.
I definitely agree that there are a few utility-type functions that I’ve either copied and pasted from the core lib or written from scratch and subsequently realised that I’d reimplemented something in that was already in the core library.
Also – there’s no reason to drastically increase the API surface area. If all of the existing commands operated on transactions and returned one, then there could be a new high level command helper dispatcher() or something like that, so dispatcher(joinBackward()) would be equivalent to the joinBackward that exists today.
You usually don’t want the exact command as a helper, though. Where a command will act on the selection, a helper tends to work better when it takes arguments that tell it where to apply its effect.
But I’m okay with growing the API surface a little to expose reusable code where it’s appropriate. Do you want to propose a pull request that splits the guts of liftListItem into an exported utility function?
Passing a selection makes sense. Or it could just be a pair of positions. Also, I’d include itemType directly in the parameters of the function—if we don’t have to correspond to the command function signature, there’s not need for the wrapping factory function.
I also run into this issue now and then, wanting to combine the functionality of several commands. However, that use case doesn’t seem to be supported since the commands immediately dispatch the transactions and I’m left with copy-pasting the code from prosemirror-commands.
I’ll try employing the pattern suggested by @marijn, but I feel that the API could be improved here to avoid code duplication. I don’t think this discussion has lead to changes in the code, has it?
I don’t know the design decisions behind the current API, but I’ll take the liberty to present the following, possibly foolish suggestion.
Looking at the prosemirror-commands source code, commands only use the state to get the current document, selection and storedMarks. I believe these can also be retrieved from a transaction and the interface of commands could possibly be changed to accept only a transaction instead of the editor state?
Considering if (dispatch) dispatch(...) is (almost) always followed by return true, perhaps commands can return a transaction when the command can be applied, or null otherwise (equivalent to a return value of false now). This does however mean that transactions are constructed, even if they are not going to be dispatched by the caller. Is that too wasteful?
This kind of interface allows chaining commands, AFAICS. Instead of changing the commands interface, an intermediate level of mutators can be introduced, as suggested by @bradleyayers. Backwards-compatible commands could be constructed automatically from these:
function createCommand(mutator) {
return function(state, dispatch) {
let tr = mutator(state.tr)
if (tr === null)
return false
dispatch(tr)
return true
}
}
That may go for the commands in that package, but is not generally true. For example, the undo/redo commands will access the state’s history field.
As I pointed out in other messages, I don’t think it is possible to compose commands, in general, and having use-case-specific helper functions that implement the difficult parts some commands would be the way to go.
You can pass this state to multiple commands with an empty dispatch function and then dispatch the tr so that all the updates are composed onto the same transaction.
Trying to wrap my head around the precise use case of calling a command from another command: can’t this be achieved by passing down a stateful dispatch function?
Something along those lines:
function myCmd() {
return cmd(state, dispatch) {
if (!dispatch) return false
const transactions = []
function nestedDispatch(tr) {
transactiosn.push(tr)
}
// using an existing command (in more complex code this might depend of state etc.)
toggleMark(someMark)(state, nestedDispatch)
const mergedTransaction = transactions
.reduce(/* merge transactions into one somehow ?*/)
dispatch(mergedTransaction)
}
}
So my question would be: can we marge transactions afterwards?
Without a pattern to compose commands, I’ve noticed our code tends to create a lot of transactions by running multiple commands where conceptually only one is expected, which may then have a weird effect on undo/redo. For instance, running some commands in a loop.
In ProseMirror’s approach, commands are actual self-contained user actions. The idea is that you write helper functions to implement the logic that you need to share between commands, and commands themselves aren’t composable. Because transactions fundamentally can’t be blindly merged—they may set metadata that breaks if additional changes or effects are included in the same transaction.
I know Tiptap is less careful about this distinction, but I suspect that will cause issues in some cases.
So for instance let say I want to implement a command that remove “bold”, “italic” and “underline” markes in one go (but keep other marks around). For this use case you would rather reuse some logic similar to toggleMark implementation to craft a new command, rather than trying to find a hack to trigger 3 toggleMark calls in the same transaction?
(trying to rephrase on a basic example for extra clarity because the discussions above are more conceptual)
Correct. Unfortunately, some of the commands in prosemirror-commands do rather non-trivial things and don’t expose related helper functions (yet). But if you see a piece of functionality that looks like it should be factored out into an exported utility function, feel free to propose that.
wrapInListTr is just the same logic as wrapInList as it currently exist in the codebase, but it takes the transation as a parameter
wrapInList takes care of initiating the transaction, but that’s all
export function wrapInListTr(tr: Transaction, listType: NodeType, attrs: Attrs | null = null): Transaction | false {
let { $from, $to } = tr.selection
let range = $from.blockRange($to), doJoin = false, outerRange = range
if (!range) return false
// This is at the top of an existing list item
if (range.depth >= 2 && $from.node(range.depth - 1).type.compatibleContent(listType) && range.startIndex == 0) {
// Don't do anything if this is the top of the list
if ($from.index(range.depth - 1) == 0) return false
let $insert = tr.doc.resolve(range.start - 2)
outerRange = new NodeRange($insert, $insert, range.depth)
if (range.endIndex < range.parent.childCount)
range = new NodeRange($from, tr.doc.resolve($to.end(range.depth)), range.depth)
doJoin = true
}
let wrap = findWrapping(outerRange!, listType, attrs, range)
if (!wrap) return false
//if (dispatch) dispatch(doWrapInList(state.tr, range, wrap, doJoin, listType).scrollIntoView())
return doWrapInList(tr, range, wrap, doJoin, listType).scrollIntoView()
}
/// Returns a command function that wraps the selection in a list with
/// the given type an attributes. If `dispatch` is null, only return a
/// value to indicate whether this is possible, but don't actually
/// perform the change.
export function wrapInList(listType: NodeType, attrs: Attrs | null = null): Command {
return function (state: EditorState, dispatch?: (tr: Transaction) => void) {
const tr = wrapInListTr(state.tr, listType, attrs)
if (!tr) return false
if (dispatch) dispatch(tr)
return true
}
}
and here is how it can be used to swap list type (removing previously set list type, and applying wanted one) in a single command:
function swapListType (state, dispatch) {
// remove other type of list if any
let tr: Transaction | false = state.tr
for (const markType of listNodes) {
if (innerNode.type === markType) {
// note that I have refactored "liftListItem" too
// to accept a transaction
tr = liftListItemTr(tr, li)
if (!tr) return false
break
}
}
// apply wanted type
tr = wrapInListTr(tr, mt)
if (tr) {
dispatch(tr)
return true
}
return false
}
—>
