tiptap icon indicating copy to clipboard operation
tiptap copied to clipboard

dispatch=undefined in createCan() results in dispatch attempts during can()

Open kivikakk opened this issue 3 years ago • 0 comments

What’s the bug you are facing?

I've hit a weird issue that requires a combination of three things:

  1. A custom node that nests another custom node, which in turn contains inlines. a. It needs to have higher priority than Paragraph.
  2. editor.can().toggleBulletList() (or editor.can().toggleOrderedList())
  3. A selection from within a paragraph after the custom node into the inline in the custom node.

Which browser was this experienced in? Are any special extensions installed?

Latest Chrome on macOS, but unlikely to be browser-specific.

How can we reproduce the bug on our side?

Here's a sandbox those shows the issue: https://codesandbox.io/s/tiptap-dispatch-repro-gc54w8?file=/src/App.js

Try making a selection like this:

image

editor?.can().toggleBulletList() will cause a RangeError, specifically from here:

https://github.com/ueberdosis/tiptap/blob/e2f8747e2b10f98d49798b15d64db5f0a36a3427/packages/core/src/commands/clearNodes.ts#L41-L45

The tr.setNodeMarkup call throws:

RangeError
Invalid content for node type mycontainer

See relevant PM code. At the point of error, type is the NodeType for MyContainer, and node.content is a fragment containing TextNode(s).

Can you provide a CodeSandbox?

No response

What did you expect to happen?

I'm surprised that a can() call could throw an error at all, regardless of the schema. clearNodes checks for whether it should dispatch:

https://github.com/ueberdosis/tiptap/blob/e2f8747e2b10f98d49798b15d64db5f0a36a3427/packages/core/src/commands/clearNodes.ts#L20-L22

But in this case, dispatch is () => void 0. That value comes from CommandManager.buildProps:

https://github.com/ueberdosis/tiptap/blob/e2f8747e2b10f98d49798b15d64db5f0a36a3427/packages/core/src/CommandManager.ts#L141-L143

shouldDispatch is true. This is surprising to me. Here's the relevant part of CommandManager.createCan:

https://github.com/ueberdosis/tiptap/blob/e2f8747e2b10f98d49798b15d64db5f0a36a3427/packages/core/src/CommandManager.ts#L108-L112

dispatch is set to undefined. Even explicitly passed undefined arguments are subject to defaults (today I learned :/ apparently JavaScript can still surprise me), which means the this.buildProps(tr, dispatch) call gets shouldDispatch set to true.

A year and a half ago in ca8d1a4245 the dispatch value in createCan was changed from false to undefined:

image

That led to this state of affairs, I think.

Anything to add? (optional)

The following appears to fix this issue, and all tests pass locally, so I'll open a PR:

diff --git a/packages/core/src/CommandManager.ts b/packages/core/src/CommandManager.ts
index c1c62ffcc..7e4dd561d 100644
--- a/packages/core/src/CommandManager.ts
+++ b/packages/core/src/CommandManager.ts
@@ -107,13 +107,13 @@ export class CommandManager {

   public createCan(startTr?: Transaction): CanCommands {
     const { rawCommands, state } = this
-    const dispatch = undefined
+    const dispatch = false
     const tr = startTr || state.tr
     const props = this.buildProps(tr, dispatch)
     const formattedCommands = Object.fromEntries(Object
       .entries(rawCommands)
       .map(([name, command]) => {
-        return [name, (...args: never[]) => command(...args)({ ...props, dispatch })]
+        return [name, (...args: never[]) => command(...args)({ ...props, dispatch: undefined })]
       })) as unknown as SingleCommands

     return {

Did you update your dependencies?

  • [X] Yes, I’ve updated my dependencies to use the latest version of all packages.

Are you sponsoring us?

  • [X] Yes, I’m a sponsor. 💖

kivikakk avatar Jul 26 '22 05:07 kivikakk