Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propose history tracers #12

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions text/0000-history-tracers.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# Summary

This proposal adds features to the [prosemirror-history](https://prosemirror.net/docs/ref/#history) package that allow you to see when a given step is undone or redone.

# Motivation

The undo history currently strictly concerns itself with the document—undoing a change means undoing the steps that were made as part of that change, and nothing more. But some interfaces don't make such a clear distinction between actions that change the document and actions that change some other piece of state—possibly ProseMirror plugin state, possibly entirely external state—and would like to use the same undo/redo commands to go through document changes and external state changes.

In some cases, changes in external data are tied to document steps (for example, creating a node for which some metadata is tracked outside of the document), in other cases, the changes are entirely independent of the document.

The current prosemirror-history implementation makes it extremely hard to extend the history in this way. This proposal intends to add functionality to make this easier.

# Guide-level explanation

"Tracers" are values that can be associated with a given step to provide additional information about it, and track that data through undoing and redoing the step. They are stored as transaction [metadata](http://prosemirror.net/docs/ref/#state.Transaction.setMeta).

When you create a transaction that contains some traceable steps, you use the `tracers` metadata key to add this information to the transaction. It holds an array of `Tracer` objects, each of which has an index that points at a step in the transaction, a tag that identifies the kind of tracer it is, and optionally an additional value of arbitrary type that provides further information.

The undo history stores these tracers along with the steps, and When it creates a transaction that undoes or redoes some steps, it includes the relevant tracers in that transaction. An `event` property on tracers can be used to see what kind of transaction this is—it initially holds `"do"` in the user-created transaction, `"undo"` when the transaction undoes the step, and `"redo"` when it redoes it again.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't part of the scope of this RFC as it stands, but I think Tracer seems like it could also be a nice fit for the rebase operation that happens in prosemirror-collab as well; they both mainly concern themselves with steps, and both can have the effect of replaying steps that may or may not have trace-able data in them. Just a thought!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, that's a good point. Just to check if we're on the same page, at a minimum this would involve:

  • Moving tracers into a more central package, probably prosemirror-state

  • Having the collab module add tracers with a type like "rebase" when it rebases traced steps

Can you sketch a situation in which such information would be used?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes....some pseudo-code for a hypothetical transaction utility function that wanted to consume/produce such things:

import { tracers } from 'prosemirror-tracer'

function wrapWithMetadata(tr, userId, isRebaseable) {
    let tracers = tr.getMeta(tracers)
    let traceData = []
    tr.steps.forEach((step, i) => {
        traceData.push(new Tracer(i, "stepMetaData", {
            userId,
            isRebaseable,
            createdAt: Date.now()
        }))
    });
    if (!tracers) {
        tr.setMeta(tracers, traceData))
    } else {
        tracers.forEach(trace => {
            // we copy the trace data for collab rebases
            // and redoes/undoes UNLESS the tracer metadata
            // specifically designates us not to
            if (!tr.steps[trace.index] instanceof NoOpStep || !trace.value.isRebaseable || ['undo'].includes(trace.event)) {
               traceData[trace.index].value = trace.value;
            }
        })
    }
    myCustomBackend
        .send(traceData)
        .then(() => console.warn('per-transaction metadata sent'))
}

In this particular implementation, the idea is that depending on some combination of the trace data itself, and the union type of the event (which in my thinking could come from prosemirror-collab vs prosemirror-history....not sure the best way to represent this), determines whether or not we copy the trace data that came from the original transaction, vs. using the "fresh" trace data.

I'm not totally sure the best way to represent the event to allow for both history/collab...maybe this could also be namespaced to the plugin that ends up copying/preserving the trace data?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Would it make sense for the collab module, when it rebases steps, to add tracers for those steps twice—once when it reverts them at the start of the rebase, and once when it re-applies them at the end?

I think, since the number of different actions that involve re-applying past steps is quite limited, we should be fine with just using strings for the event types.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this revision look to you?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good; so for the average rebase, where a step isn't dropped, you'd end up with two traces — one for rebase-invert and rebase-reapply?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. That's slightly awkward, in that you have to check whether both or only one tracer is present for a step to determine what happened to it, but it seems the most straightforward way to describe what's going on.


# Reference-level explanation

This change requires the history module to track a set of tracers along with each step. Tracers are preserved through mapping (and rebasing), so the step that a tracer eventually ends up with is not guaranteed to be identical to the one it started with—they track step origins, but mapping and reordering (through `"addToHistory": false` transactions) might cause the step to change shape.

When a step is dropped because it cannot be mapped through `"addToHistory" false` steps coming after it, the tracer is dropped. This seems like reasonable behavior: if a step can't be undone, it should not be reported as undone.

One tricky part is support for actions that do not have steps associated with them. The current architecture of the history package is very much built around steps, and adding support for actions that aren't associated with steps, while possible, would further complicate an already difficult piece of code. As such, my proposal is to lean on support for custom step types to sidestep this problem something like this:

- Create a custom subclass of [`Step`](http://prosemirror.net/docs/ref/#transform.Step).

- Make it do nothing, have an empty [map](http://prosemirror.net/docs/ref/#transform.StepMap), and return itself when mapped—or, if it can be meaningfully associated with some point in the document, it could fail to map through mappings that delete that point.

- Include such a step in your transaction, and associate the tracer with it.

This is slightly awkward, but works robustly. We can consider including a no-op step type in the prosemirror-transform package to make it easier.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case it's informative, our team at the New York Times has already done exactly this, and it has worked well for exactly these non-mutating use-cases you describe.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's helpful to know, thanks.


# Drawbacks and alternatives

The clumsiness around step-less action could be one motivation to do this differently.

Arguments can be made for doing this at the granularity of transactions instead of steps, but since the history tracks steps, not transactions, and it is possible for some of the steps in a transaction to be canceled by remapping, that would create a lot of difficult corner cases that are sidestepped by explicitly making the user select steps to track.

I initially considered associating imperative callbacks with steps, which are called for their side effects when the step is undone, but that would appear to be a terrible fit with the rest of our architecture (building a transaction shouldn't have side effects). Putting the metadata in the transactions and having other code react to those (with a plugin state apply method when working inside of the editor state, or `dispatchTransaction` otherwise) nicely avoids those issues.