Request: View plugin method that runs at the same phase as update listeners

v6
1

At Replit, we have this pattern which is used in a lot of our plugins:

class MyPluginImpl implements PluginValue {
  postUpdate(update: ViewUpdate) {
    // safe to do dispatches here
    if (whateverCondition) {
      update.view.dispatch({ /* ... */ })
    }
  }
}

const myPlugin = ViewPlugin.fromClass(MyPluginImpl, {
  provide: (v) => EditorView.updateListener.of((update) => {
    update.view.plugin(v)?.postUpdate(update)
  }
})

postUpdate is helpful in a lot of situations. It enables you to make update listeners that have internal state, without using something like closures.

For us, it’s often needed because an extension needs to manage some part of the editor’s state, but that management logic just can’t go into something like a StateField. Handling asynchronous state is usually the reason why - often an update to the editor means that some asynchronous process that was running needs to be aborted, started, or restarted. That process may have some information about it stored in EditorState, e.g. whether or not it’s running/loading, so we may need to do a dispatch when we start/abort/restart the process. This is our most common use case, e.g. our inline AI suggestions use this. Most of the suggestion state is in a StateField, but most of the logic for updating that field lives in a view plugin because that’s the only reasonable place to do asynchronous requests.

Another use case for us is updating things outside the editor, which then may have reactive effects which cause editor dispatches. It is desirable to avoid this entirely, but for our React-inside-of-CodeMirror library, we definitely needs this, or at least workarounds similar to this.

We could do something like queueMicrotask(() => view.dispatch({ ... }) instead of using an update listener, but this has issues with making view updates sort of slightly asynchronous. As-in, if you called view.dispatch, and then immediately accessed view.state afterwards, that state won’t yet reflect anything queued up by queueMicrotask yet. Using update listeners avoids this, by basically letting a single dispatch turn into multiple dispatches.

So, our request here is pretty simple: add a method very similar to update that runs after update and only when it is safe to do dispatches to the view, just like update listeners. The only real drawback I can think of to adding this method is maybe confusion about what method to use when learning the API, and the possibility of infinite update loops. The latter is already possible with update listeners, so IMO it’s fine.

2 Likes
2

In my own code I’ve been pretty successful at avoiding this pattern where a dispatch immediately triggers another dispatch. Cascading updates are a bit wasteful, in that they run through the view update logic multiple times for what should be an atomic update. You can often set things up so that the state update plans the asynchronous actions, and a plugin update method actually activates/schedules them, without updating the state. Or in some cases you can dispatch multiple transaction at once by passing them to dispatch as an array.

Note that even though the code after the dispatch would see the updated state if you had this feature, the update listeners or postUpdate hooks will have situations where the update they are looking at is not actually the most recent update anymore, because another such listener dispatched a transaction. I sort of feel that doing this kind of cascading dispatch asynchronously might be less error-prone than doing it synchronously.

3

It is best to avoid this pattern, yes. Honestly, I’m kind of advocating for this for two main reasons:

  • for some extensions, it’s the easiest way (that’s mostly effective) to handle a problem
  • it’s helpful in weird scenarios, such as when you need to make sure that your handler runs after some other extension’s handlers, or when dealing with tricky lifecycle stuff (like integrating UI frameworks)

You can usually avoid it, and you should if you can. But it’s still useful for us. It feels like a missing lifecycle method with view plugins. I will say not every use case we have for this is just about dispatching.

4

Would just adding something like this to your plugin spec work for you? I’m not keen on steering people in the direction of messy imperative state cascades by making ‘plugin lifecycle methods’ too much of a thing.

  provide: plugin => EditorView.updateListener.of(update => {
    update.view.plugin(plugin)?.postUpdate()
  })
5

That’s what we’ve been doing so far. It feels a bit hacky, which is why I made this request - but then again, I guess what we’re doing with this feature is a little hacky.

I took another look at our uses of this pattern (which is not too many, really), and I’ve noted the following:

  • We sometimes use it because it makes the logic of the extension much easier to understand, usually because it lets you organize your extension in a nice pattern that looks something like:

    • a state field, which is only concerned with being updated by an effect and mapping its current value across transactions (e.g. a range set),
    • and a view plugin, which is only concerned with updating the state field based on the plugin’s internal state.

    We have a lot of messy, mutable services that we need to pass into plugins, e.g. a service for making filesystem related calls. Passing these services into the state seems “wrong”, since an editor state is immutable and services are both mutable and may be destroyed at any time. But we need these services to know how to update state, so the service and the update logic ends up in a view plugin that may use a pattern like this.

    Of course, this use case doesn’t invalidate what you said, you could still avoid postUpdate regardless with enough work. It’s just messy, and it’s hard to think about how to handle cases where there is editor state that is derived from one of these services.

  • We sometimes use it to guarantee that something runs after something else, without having to resort to precedence. This is uncommon. We just have a lot of extensions, and so it’s hard to depend on precedence for everything.

  • It’s helpful for integrating with UI frameworks in a safe (ish) way. You might have e.g. React components that depend on editor state, but you don’t know if updating them will cause them to dispatch something to the editor (it’s a very complex app, and we often have to reconfigure the editor with different extensions and we have a whole framework for doing this). Doing the component update inside of postUpdate means even if the component dispatches to the view, it’s safe.

  • A usage that I find interesting is our “observation mode”. We have a multiplayer editor, so we have this feature where you can observe a user and follow their cursor in the editors they have open. We have a state field that holds peer’s cursors, and our observation mode plugin listens to that field and dispatches EditorView.scrollIntoView effects when the observed cursor moves.

    It would be possible to avoid using this pattern here I think, via a transactionFilter or transactionExtender maybe. It would make the extension harder to understand though, since the plugin would still need to handle dispatching scroll effects sometimes, and it would require adding another state field that holds the currently observed user that the filter then checks. It makes the entire extension much more complicated and also adds the potential footgun of checking tr.state in the filter.

  • Something in general that we struggle with is that we sometimes need extensions that are in some way derived from the editor state (or the view’s state). e.g. we have an extension which restores your undo history, scroll position, and folded ranges when you open a file. This extension must wait on the file to first be fetched and inserted into the editor’s document, so it waits on a file-is-done-loading field to be true before it reconfigures the editor and mounts the history extension.

    Like before, there are ways to avoid needing postUpdate here, but they would mostly involve waiting to construct the editor until we had everything we needed. That’s undesirable for us for quite a few reasons. We like being able to fully describe our editor using just extensions.

I’ll let you be the judge of whether or not our use cases are justified here - but altogether I feel like it’s often easier to just compromise and use this method. Commenting on the API, I feel like engineers struggle to know:

  • what should go into state field/facet versus a plugin
  • how to handle asynchronous code
  • how to update state from a plugin “correctly”
  • when it’s safe to dispatch in a plugin, or if you should ever dispatch (you should, but not being able to do so inside of update and the plugin constructor is not intuitive)
  • how to compose plugins, fields, and facets into a cohesive extension
6

It somewhat sounds like you’re really fighting the design of the library in some of these cases, going for imperative patterns though the API is trying really hard to avoid the need for those.

I’d say you clearly would want to save information about the cursor you are following in a state field. Where else are you storing it? The whole thing seems like it would be very straightforward to do with a transaction extender. Is the case where you’d still need to dispatch the point where a new user is selected as being observed? It seems like that could also be handled by the same extender.

It seems you’re not creating a fresh state when loading a new document? That sounds like it is going to complicate things, yes. But it’s usually possible to model loading a document as a set of asynchronous actions that fetch all the necessary data, some logic that creates a complete editor state from that, and a call to setState. That requires special handling of state resets in editor-external code that’s observing the editor updates, but in my experience trying to model reloads through state updates also requires that, just in an easier-to-miss way.

You indeed don’t want to connect mutable external services directly to the editor state, and I agree that solutions that neatly integrate them into the editor update lifecycle can get a bit baroque. I don’t know your specific use cases, so I can’t say just how bad they’d get, but I do feel that trying to do that integration anyway is usually a good idea.

I spent a lot of effort trying to make it so that the system can avoid the imperative-cascade-of-updates pattern, since that leads to a lot of wasted CPU work and subtle bugs around state inconsistency and update timing. I’m aware that this can be a hard system to fit some types of features into. I do think it’s worth doing anyway, though.

7

It somewhat sounds like you’re really fighting the design of the library in some of these cases, going for imperative patterns though the API is trying really hard to avoid the need for those.

I think this is mostly what it is. It’s easiest to reach for imperative patterns.

I’d say you clearly would want to save information about the cursor you are following in a state field. Where else are you storing it?

The selected cursor is just a prop in the view plugin’s class. It really could be in a state field, but doing this:

class MyPluginImpl {
  private followedCursor: Cursor | null  
  // ...
  update(update: ViewUpdate) {
    if (this.followedCursor && update.docChanged) {
      this.followedCursor = this.followedCursor.map(update.changes)
    }
  }
}

export const MyPlugin = ViewPlugin.fromClass(MyPluginImpl)

is much more obvious than doing:

const updateFollowedCursor = StateEffect.define<Cursor | null>({
  map: (value, changes) => value.map(changes)
})

const followedCursor = StateField.define<Cursor | null>({
  create: () => null,
  update: (value, tr) => {
   let updated = false

    for (const effect of tr.effects) {
      if (effect.is(updateFollowedCursor)) {
        value = effect.value
        updated = true
      }
    }

    if (!updated && value && tr.docChanged) {
      value = value.map(tr.changes)
    }

    return value 
  }
})

class MyPluginImpl {
  // ....
}

export const MyPlugin = ViewPlugin.fromClass(
  MyPluginImpl,
  { provide: () => followedCursor }
)

// you could also mount the plugin via the field
// also this is probably the wrong way to do it, easier to track the username,
// then find it in the list of cursors, but that's not the point

I’ve thought about it, and I think a lot of our issues revolve around this. It’s a lot of code - a disconcerting amount actually - just to track the followed cursor, and that’s not including the transaction extender. I get why you’d want to do it this way, there are tons of reasons, but it’s hard to get into the right headspace to organize state like this. I’m quite well versed in the API at this point, but most of my team isn’t and I struggle to make this more spread out code feel readable to others.

Also, what should be in a state field versus something internal to the plugin is sometimes hard to determine.

The whole thing seems like it would be very straightforward to do with a transaction extender. Is the case where you’d still need to dispatch the point where a new user is selected as being observed? It seems like that could also be handled by the same extender.

I’ve thought about this too, and I feel like I avoided an extender here because:

  1. I didn’t think about using them too much, because of a reason I’ll get to later, and,
  2. the only usage of an extender (which I did not author) we had in our code base caused performance problems and I personally got rid of it

The potential footgun with tr.state is so explosive that I felt naturally compelled to avoid it, perhaps irrationally. Just thinking out loud: maybe an eslint rule that warns on accessing tr.state in an extender could help ease the anxiety there.

It seems you’re not creating a fresh state when loading a new document? That sounds like it is going to complicate things, yes. But it’s usually possible to model loading a document as a set of asynchronous actions that fetch all the necessary data, some logic that creates a complete editor state from that, and a call to setState.

We didn’t use setState or think about constructing an editor state like that because honestly, we did not get inspired enough to realize we could use it like that. I often look at the official extensions like autocomplete or lint to get reference on how the CodeMirror APIs were intended to be used, and features like setState, transactionExtender, and inputHandler come up so infrequently that I don’t really have them in my “bag of patterns”, so to speak.

It’s still not the perfect pattern for us due to the external code thing - a lot of which has to do with integrating with React, which is frustratingly hard and I mostly blame React for it.

You indeed don’t want to connect mutable external services directly to the editor state, and I agree that solutions that neatly integrate them into the editor update lifecycle can get a bit baroque. I don’t know your specific use cases, so I can’t say just how bad they’d get, but I do feel that trying to do that integration anyway is usually a good idea.

Honestly, the most annoying part is just getting them into plugins, and being able to reload those plugins if that service is recreated, destroyed, or whatever. This is also related to React - gracefully updating the editor is tricky in it.

I think with your reply you’ve mostly convinced me that adding something like postUpdate goes too hard against the point of the API. I’m going to think about maybe what could be added to the APIs that would make them easier to use correctly, e.g. making defining an extension’s state a little easier.