⚠️ No Changeset found

Latest commit: 83e48ff

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

✅ Deploy Preview for tiptap-embed ready!

Name	Link
🔨 Latest commit	83e48ff
🔍 Latest deploy log	https://app.netlify.com/sites/tiptap-embed/deploys/671a34ee0bae970007fde2a1
😎 Deploy Preview	https://deploy-preview-5744--tiptap-embed.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Hey @guanriyue I haven't reviewed this super thoroughly, because I think that this approach is wrong.

What can is meant for is to be able to allow commands to check whether they would be able to execute without actually modifying the document via transactions. At times, this can be sort of ambiguous, so sometimes a lazy implementation may not actually implement this properly. I think that clearNodes in this case is just implemented improperly where it should not have that early dispatch.

I do not believe that you need to actually change can in this case. I think that what needs to happen is there needs to be a better implementation of clearNodes.

Have you looked at the description of this issue #5721? I explained why I believe dispatch should be a function in can.chain.

Perhaps it is a mistake; in fact, setting dispatch to true was part of the code logic before the last bug fix #3026. It's actually a bit of a long story.

Four years ago, at the very beginning, the dispatch variable was set to false.
7bfab46#diff-ac93a7861b6cdca5f5dd83ec422e904ebb145fd00a39343834384c6d241965c6R103

About five months later, it was changed to undefined for unclear reasons. When passed into buildProps, JavaScript treated it as true by default. These two changes come from the same author.
ca8d1a4#diff-ac93a7861b6cdca5f5dd83ec422e904ebb145fd00a39343834384c6d241965c6L87-R90

About a year and a half later, clearNodes caused an error, but the issue with clearNodes was not fixed, and executing command.clearNodes could still lead to problems, while the implementation of createCan was modified. dispatch was reset to false.
40f4ea3#diff-ac93a7861b6cdca5f5dd83ec422e904ebb145fd00a39343834384c6d241965c6L109-R111

After that fix, several new issues with can checks emerged.

• .can() returns false for extension-list even when the command is runnable #3229
• [Bug]: can command not working correctly with list #4057

I am now trying to change it back to true . And fix the real issue in clearNodes. It runs all the tests successfully on my local machine, except for the can.chain.dispatch.except.to.be.undefined, which is a test case added after the fix #3026.

If dispatch is true, commands will think that they can modify the transaction when they should not be able to.

Yes, you're right. However, in can.chain, it seems that we should not skip modifications on tr under any circumstances, as subsequent commands may depend on the state and tr after the preceding command is executed. Just like how skipping clearNodes caused issues. There are many commands similar to clearNodes. In our pursuit of higher performance and avoiding unnecessary code execution, we tend to favor lazy implementations.

dispatch should probably be named shouldDispatch where it being false means you should not, as a command, actually apply your change to the editor.

Yes, that is an inappropriate naming; it should be renamed to shouldDispatch.

I'm sorry, but, I still believe that this is all misguided. Please refer to the documentation of Prosemirror https://prosemirror.net/docs/guide/#commands It specifically says that:

To figure out whether a selection can currently be deleted, you'd call deleteSelection(view.state, null), whereas to actually execute the command, you'd do something like deleteSelection(view.state, view.dispatch). A menu bar could use this to determine which menu items to gray out.

In this form, commands do not get access to the actual editor view—most commands don't need that, and in this way they can be applied and tested in settings that don't have a view available. But some commands do need to interact with the DOM—they might need to query whether a given position is at the end of a textblock, or want to open a dialog positioned relative to the view. For this purpose, most plugins that call commands will give them a third argument, which is the whole view.

function blinkView(_state, dispatch, view) {
if (dispatch) {
view.dom.style.background = "yellow"
setTimeout(() => view.dom.style.background = "", 1000)
}
return true
}
That (rather useless) example shows that commands don't have to dispatch a transaction—they are called for their side effect, which is usually to dispatch a transaction, but may also be something else, such as popping up a dialog.

It specifically says it should be called like deleteSelection(view.state, null), with the implementation as:

 function blinkView(_state, dispatch, view) {
  if (dispatch) {
    view.dom.style.background = "yellow"
    setTimeout(() => view.dom.style.background = "", 1000)
  }
  return true
 }

This is done purposefully, so that a command can be "tried" without actually committing the side-effect. If you pass true you defeat the purpose of it as you'll modify the editor content with the transaction or side-effect.

You are not solving the real problem which is the implementation of the command. You are attempting to change how can chains work to fit a wrong implementation, fix the implementation not how can chains work.