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
from

Conversation

Projects
None yet
4 participants
@marijnh
Copy link
Member

commented May 16, 2019

This proposal adds features to the prosemirror-history package that allow you to see when a given step is undone or redone.

An initial implementation exists in this branch.

Rendered RFC

@marijnh marijnh force-pushed the history-tracers branch from 7a30224 to 870f07f May 16, 2019

@saranrapjs
Copy link

left a comment

This would be super useful!


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

This comment has been minimized.

Copy link
@saranrapjs

saranrapjs May 16, 2019

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.

This comment has been minimized.

Copy link
@marijnh

marijnh May 17, 2019

Author Member

That's helpful to know, thanks.


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.

This comment has been minimized.

Copy link
@saranrapjs

saranrapjs May 16, 2019

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!

This comment has been minimized.

Copy link
@marijnh

marijnh May 17, 2019

Author Member

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?

This comment has been minimized.

Copy link
@saranrapjs

saranrapjs May 20, 2019

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?

This comment has been minimized.

Copy link
@marijnh

marijnh May 20, 2019

Author Member

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.

This comment has been minimized.

Copy link
@marijnh

marijnh May 20, 2019

Author Member

How does this revision look to you?

This comment has been minimized.

Copy link
@saranrapjs

saranrapjs May 20, 2019

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?

This comment has been minimized.

Copy link
@marijnh

marijnh May 22, 2019

Author Member

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.

@bradleyayers

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

My use-case for this would be tracking ranges over a document. Presumably I'd store the range information in plugin state, and then update when the plugin is given the transaction. I think there should be enough information available through tracers to do this. Having a NoOp step doesn't phase me, it seems like a reasonable compromise.

@marijnh

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

My use-case for this would be tracking ranges over a document.

What kind of ranges, and how do individual steps come into that? Is this something like adding a comment/annotation, and being able to undo and redo that?

@marijnh

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

Following discussion with @saranrapjs , I've updated the proposal to increase its scope a bit—the collab module could also use tracers to provide more information about rebased steps.

@vladminsky

This comment has been minimized.

Copy link

commented May 24, 2019

My use-case for this would be tracking ranges over a document.

What kind of ranges, and how do individual steps come into that? Is this something like adding a comment/annotation, and being able to undo and redo that?

@marijnh just FYI my particular use case looks similar to what you described.

Comments plugin is implemented using decorations. Once a user deletes a text with a comment decorations and then undo deletion - text is restored while comments are lost.

@marijnh

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

I see. That case wouldn't be straightforwardly covered by this feature, unfortunately—the steps that delete the commented text are likely created by code that doesn't know about comments, and thus won't be adding the tracers to indicate that a given step deletes a comment (which you could then use to restore it when the step is undone).

That is a common use case though, and we should probably try to incorporate it in the proposal. It seems like it would be necessary for plugins to somehow add tracers to any transaction that comes in, maybe through a hook that's called right after the transaction filter.

That seems a bit random, and I don't have time today to think about it more deeply, but I guess it should be safe (adding trace metadata shouldn't conflict with any other functionality).

@marijnh

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

Patch 6a69d90 adds a plugin hook attachTracers that can be used to add tracers to transactions coming from other sources.

@@ -18,6 +18,8 @@ When you create a transaction that contains some traceable steps, you use the `t

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.

This comment has been minimized.

Copy link
@saranrapjs

saranrapjs May 30, 2019

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)

This comment has been minimized.

Copy link
@marijnh

marijnh May 30, 2019

Author Member

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.

@marijnh

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

There's now an implementation of the current state of the RFC in the tracer branches of the state, history, and collab repositories.

If you're interested in this, it'd be wonderful if you could try it out and see if it actually works for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.