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.

Sign up for GitHub

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

Propose history tracers #12

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
870f07f
Propose history tracers
marijnh May 16, 2019
36ae1c4
Update tracer proposal to make tracers cover more than just history
marijnh May 22, 2019
6a69d90
Add the 'attachTracers' hook to the tracer RFC
marijnh May 30, 2019
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
49 changes: 49 additions & 0 deletions text/0000-history-tracers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# Summary

This proposal defines a new, optional, piece of transaction metadata that can be used to trace steps through history undo and redo, or collab rebasing.

# 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. Similarly, the way prosemirror-collab can re-apply local steps several times can make it hard to keep track of such steps.

# 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.

Both the `Tracer` class and the `tracers` metadata key are exported from prosemirror-state.

To cover the use case where tracers need to be attached to steps by code that didn't originate those steps, for example when you want undoing the deletion of content that had some metadata associated with it to restore that metadata, plugins support a new hook, `attachTracers`, which is called right after `filterTransaction`, and given a transaction may return an array of additional tracer objects to associate with the transaction.
Copy link

Choose a reason for hiding this comment

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

To be clear: when undoing steps that have tracers associated with them, the existing spec already will re-add tracers, but this new capability is to support cases where the metadata may be specific to the content of the steps? Is attachTracers implemented somewhere we can see? (mostly just trying to get a sense for whether we need this vs. if just attaching tracers when dispatching the transaction will suffice)

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this hasn't been implemented yet. But basically, the transaction apply driver will, before applying each transaction, see if any plugins want to add tracers, and if so, add them to the transaction's existing set of tracers. So any downstream code (such as state field apply methods) will see the full set of tracers.

That does mean that attachTracer callbacks may in some cases have to check whether a tracer already exists—for example if you delete a range that had a comment associated with it, a plugin might add information about that comment as a tracer. If that step is then undone and redone, the plugin will see the same step again deleting a comment, but since it still has that initial tracer, you probably don't want to add another instance of it. That's awkward, but, I guess, workable.


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.
Copy link

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?

Copy link

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?

Copy link

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.


The collab module, when it rebases local traced steps over remote steps, will first apply the inverse of the steps with an event type of `"rebase-invert"`, and then, after the remote steps (if the step can still be applied) re-apply them with a type of `"rebase-reapply"`.

# 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.
Copy link

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.