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 1 commit
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
2 changes: 2 additions & 0 deletions text/0000-history-tracers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
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"`.
Expand Down