Immutable Tranform/Transaction

1

Hey everyone,

As we know, ProseMirror Transform/Transactions aren’t immutables. I always found this kind of weird, but I never saw a big problem with that until last week.

I found a dangerous loophole on ProseMirror data flow:

Any plugin can mutate the transaction during the Plugin.applyTransaction flow. That means the rootTr may not be the same for all PluginState, and worse, the state.doc can be different from the current EditorState.

This behavior is quite dangerous if your plugin relies on transactions steps like the collab plugin.

Due to the possible impact of this issue, I am spiking ImmutableTransaction. However, today isn’t 100% possible because of this single method: Transform.maybeStep. That is the only method that I can’t override.

And ProseMirror already have some code relying on this method, like: Collab and History. Sign in to GitHub · GitHub

So, I have two suggestions:

  1. A breaking change into the prosemirror-state and prosemirror-transform to make both Transaction/Transform immutables
  2. A new immutable transform class implementing the current interface AND the depreciation of the maybeStep function.

What do you all think?

2

This is more of a “just don’t do that” thing, by convention, just like immutability of the other objects is not enforced but just something your code should respect—the code that builds up a transaction can mutate it, once it is dispatched it should be treated as immutable. None of ProseMirror’s own code does this (not sure what you thought you found, but the code search you linked doesn’t show anything like that).

3

This is more of a “just don’t do that” thing

I agree, 100%. But, this makes hard to scale our code base. That problem was found in a internal (not released) code. However, it is too easy to make this kind of mistake and the result is too dangerous.

What about send a “immutable” transaction copy to those methods: apply, appendTransaction and filterTransaction.

4

What about the Immutable transaction/transform idea?

5

This code, by mistake, was merged into a working branch:

function checkSomething(tr) {
  try {
    tr.replaceRange(....);
    tr.doc.check();
  } catch (e) {
    return false;
  }

 return true;
}

And later, people thought that function was a helper one, then this happened:

new Plugin({
  state: {
    apply(tr) {
      checkSomething(tr);
    },
   },

});

I know, this is just wrong and we shouldn’t allow this kind of code to be merged. But, we are humans and we do mistakes all the time. As result of this bad code, now there is a “ghost step” inside of that transaction. The collab plugin will wrap those steps into a Rebaseable object.

Let’s say you send those steps to a server, if you don’t validate the steps on the backend, any real next Step create won’t match if the “ghost step” and then you can end up with a “unrecoverable” document.

It wouldn’t be a problem to small code base, but we have more than 30 custom (internal) ProseMirror plugins.

6

If there ever is another new major ProseMirror version, that’ll probably have TypeScript-enforced immutable transactions like CodeMirror 6 does. But until then (and there are no concrete short-term plans in that direction), I’m not changing the API in a breaking way, and you’re just going to have to be careful about this.

7

Gotcha, thank you for your time!

I think I will apply a proxy in the transaction during the dispatchTransaction flow. If any plugin tries to modify it, I will throw an error.