[Bug]: onUpdate fires when no content update has occurred

[Nantris]

Which packages did you experience the bug in?

core

What Tiptap version are you using?

2.1.12

What’s the bug you are facing?

This makes listening to the editor for updates extremely tedious and expensive in terms of both memory and CPU since we need to store an external reference and then compare against it.

What browser are you using?

Chrome

Code example

No response

What did you expect to happen?

onUpdate should not fire at editor creation time, but it does.

Anything to add? (optional)

No response

Did you update your dependencies?

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

Are you sponsoring us?

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

[bdbch]

Could you add more infos?

• Do you use collab?
• When do these onUpdate events occur?
• Was this introduced at 2.1.12 or did you experienced this earlier?
• Are there any new extensions you are using which could cause those update calls?

https://codesandbox.io/s/rough-butterfly-k2t855?file=/src/App.js

This minimal setup doesn't seem to have any unnecessary onUpdate calls so I wonder if it's an extension that was introduced.

[Nantris]

Thanks for the speedy reply and the codesandbox @bdbch. It's quite strange.

• No collab
• At startup only - when I'd expect only onCreate to run. onUpdate never runs errantly besides that
• I only started wiring things up to an external data source with 2.1.12` so unfortunately I can't say.
• I disabled all of the extensions besides StarterKit.

I also thought it might be due to StrictMode but apparently that's not the case. It's possible I made a mistake and didn't really disable everything that might interact with it but I disabled all extensions and only set a content and the on[Event] functions on the editor as far as I know during my test.

I'll have to loop back on this I guess. Thanks again for the speedy reply!

[Nantris]

Indeed I cannot repro this today. I don't know what the issue was. I'll re-open if I discover actionable details. In the meantime I'm using this alternative:

onTransaction: ({ editor, transaction }) => {
        if (transaction.docChanged) onUpdate({ editor });
      },

[Nantris]

Whoops a flaw in my testing. I seem to have discovered the cause. Running editor.setEditable and setting the same value as the existing one provokes onUpdate to fire.

Here's a repro: https://codesandbox.io/s/zealous-kate-d6s224?file=/src/App.js

[akucinskii]

I think that adding a second argument false to the editor.setEditable fixes your problem.

repro: https://codesandbox.io/s/objective-fermi-wnyrj5?file=/src/App.js

[Nantris]

Ah jeez I didn't notice that in the docs. Thanks @akucinskii

I really don't think that it makes sense to default that to true, but I'll close this issue I guess.

[Nantris]

This can occur in other cases, but I'm not really clear on the nature of those.

It seems like if modifications are made to the document that don't actually affect the serializable content it still fires. I guess this is the intended behavior but the docs could use an update.

Maybe it just checks for transaction.docChanged as I used above? I couldn't find the relevant code for onUpdate when I tried.

[jpobley]

onUpdate will also fire even if a transaction has been filtered by a plugin. Here's a minimal reproduction that filters every transaction but logs out calls to onUpdate:

https://codesandbox.io/p/sandbox/billowing-pond-2t5md7?workspaceId=a77e85f9-e56d-47f2-ae56-008fa38d6d0c

[nperez0111]

So this is the function that handles the update events: https://github.com/ueberdosis/tiptap/blob/b0124717559071729a365d67b9d4747c5fb2a1cf/packages/core/src/Editor.ts#L449-L457

I think we can make a relatively easy optimization here, if the editor.state is equal to the editor.state from before, that means none of the transactions could be applied and we can just skip calling the rest of the function.

But to be clear, this event is emitted only when the transaction does have changes or when setEditable is modified without the second parameter: https://github.com/ueberdosis/tiptap/blob/b0124717559071729a365d67b9d4747c5fb2a1cf/packages/core/src/Editor.ts#L183

For that reason, I don't think that the original issue is valid

I made a draft of this here: https://github.com/ueberdosis/tiptap/pull/5449

[jpobley]

if the editor.state is equal to the editor.state from before, that means none of the transactions could be applied and we can just skip calling the rest of the function.

We don't even need to compare the states. If we use state.applyTransaction() instead of state.apply(), we can inspect the transactions field that's returned and determine if it includes the transaction that was passed in:

const {state, transactions} = this.state.applyTransaction(transaction)
const trWasApplied = transactions.includes(transaction)

if (!trWasApplied) {
    return
}

[nperez0111]

@jpobley that is definitely useful to know! Does beg the question of what to do with appended transactions, but will look into it more later

[bdbch]

Would we need to keep track of all appended transactions and check for all of them to be included in the transactions array then?

[jpobley]

It's probably good practice and more accurate to look at the appended transactions. However, they've been ignored up until now so maybe it doesn't matter and can wait until a major rev since the behavior of dispatchTransaction() will change.

My two cents:

• Publish beforeTransactionevent
• If the transactions field returned by state.applyTransaction() is empty, then the rootTr was filtered out and we can return there.
  • Maybe still publish the transaction event but with a flag that lets the caller know it was filtered out. Not sure if that's useful to the caller.
• Otherwise, publish the transaction event.
• selectionUpdate can continue as is
• We'll then need to loop through the transactions array to determine of focus, and blur need to be published, since technically one transaction might undo a previous transaction's changes.
• Finally, it's probably easier to compare states at this point in order to determine if update needs to be called rather than trying to figure that out while looping through the transactions array

There's a question about how to publish any appended transactions. We can add an appendedTransactions field alongside the current transaction field, which would simply be transactions.slice(1). There's an argument to be made for keeping the transactions together, but this way won't be a breaking change to the event data interface. 🤷

[nperez0111]

Appreciate the thorough thoughts on this. We are actually planning on a tiptap v3 so if we want to make a breaking change I think it'd better fit there than mess with it in the maintenance version.

If you would like, feel free to make a PR with your suggestions otherwise I'll throw this in a backlog of tasks for V3 and we can get to it eventually