simplify commands, queries, and middleware

[ianstormtaylor]

Do you want to request a feature or report a bug?

Improvement / debt.

What's the current behavior?

Right now we've got three separate concepts: middleware handlers, command functions, and query functions. This is fine, but I've been bumping up against limitations with it that stem from the distinctions.

It has already been pointed out that commands and queries aren't any different from a technical point of view. It's just that some return values and some apply operations. But there's nothing stopping them from doing the other, or even one from doing both at once. (Technically commands are chainable, but this such a small benefit that shouldn't prevent better solutions.)

Also, as more and more logic is ported to editor-level queries, to allow it to be customizable in userland for more advanced editors, the special-cased middleware handlers start to seem odder and odder. For example, what is renderBlock if not a query that returns React elements? And what is onKeyDown if not a special command that is used in DOM environments.

I think we'd stand to gain in simplicity by combining these concepts.

The only difference right now is their arguments ordering, which presents a problem. Each one is slightly different. The middleware take the editor last:

(props, editor, next)
(event, editor, next)
... 

Whereas the queries and commands take it first:

(editor, range)
(editor, point)
...

---

But this brings us to a second issue that is mentioned in https://github.com/ianstormtaylor/slate/issues/2466 and in https://github.com/ianstormtaylor/slate/issues/2342 that is that the middleware stacks are awkward from a few standpoints—especially in terms of debugging.

I originally borrowed the middleware concept from Koa which seems to have solved it nicely. But having played with Micro, I've come to realize that using a next() pattern is actually just a leftover in the API from before async/await. And since we don't have the asynchronous issue—and even if we did async/await can solve it—we can eliminate the next() pattern and use full composition instead. Micro does this to a really nice effect.

The only major difference is that Micro is concerned with a single stack, whereas we need to allow a huge number of functions to be composed and overridden.

But that's not impossible, it just means that we need to use a dictionary of functions instead of a single one. Which would end up looking very similar to our existing plugin definitions for middleware handlers:

{
  onKeyDown: fn => (event, editor) => {
    if (...) {
      ...
    } else {
      return fn()
    }
  }
}

But this still leaves the problem of the ordering of editor. Since we like keeping props and event in the first position, to mimic React's event handlers and render prop patterns.

However, we can actually move the editor itself to the set of composed arguments too, since it never changes once a plugin is initialized. So we'd get:

{
  onKeyDown: (fn, editor) => event => {
    if (...) {
      ...
    } else {
      return fn()
    }
  }
}

Which leaves us with middleware, commands and queries all having the same signature:

{
  [name]: (fn, editor) => (...args) => {}
}

Meaning that plugins can be simplified to being just a dictionary of composition functions. Commands become middleware. Queries become middleware. And the React-specific commands and queries like onKeyDown and renderBlock are treated no differently.

(This requires extracting schema into a separate plugin, which we've already wanted to do from https://github.com/ianstormtaylor/slate/issues/2333.)

---

Further, since fn is actually the composed function itself, it gives us two more benefits that we are awkward with out current middleware setup:

• Changing the arguments passed to a function is clear, you can just change what you call fn(...) with, and it won't know the difference. This allows for more powerful overrides.

• Debugging is simpler, since you can step directly into the composed function, and continuing going all the way down the stack. Instead of stepping into the awkward middleware looping logic as happens now.

---

Moreover, this allows us to simplify the methods that Editor exposes. Right now since things are slightly different we have:

editor.command(type, ...args)
editor.query(type, ...args)
editor.hasCommand(type)
editor.hasQuery(type)
editor.query(type, ...args)
editor.registerCommand(type)
editor.registerQuery(type)
editor.run(type, ...args)

Which could then be simplified to:

editor.exec(name, ...args)
editor.has(name)

---

(This leaves open another question of whether it's better to have plugins be simple dictionaries of composition functions, or for them to a function that is passed the editor to use an imperative API like editor.register(name, fn). But we can choose to change this later if we discover it has benefits.)

[ianstormtaylor]

Thinking about this more...

Having a return value from commands would be useful to simplify some of the other logic in Slate's core right now, specifically how ranges and points are calculated after a command has occurred.

Right now for commands like insertFragment or splitBlock, which act on the current selection, we're forced to redo work to figure out what the new selection should be updated to. However, instead the *AtRange commands could return a range object that referred to the updated range after the command occurred.

The same could be done for *AtPoint and *AtPath commands, returning the new point and path after the command has occurred.

This would eliminate hard to follow logic in ./with-intent. And it would also helper logic in ./at-range, for example...

https://github.com/ianstormtaylor/slate/blob/67e397100e2f4cc6bcd4b8f98d7a85dcf30c72fd/packages/slate/src/commands/at-range.js#L8-L31

...would be replace by just using editor.deleteAtRange itself!

[justinweiss]

A few early thoughts:

I had a feeling something like this was coming :-) I'm converting old utils / changes to commands / queries, and they are just so similar that that distinction would eventually be removed.

Debugging plugin stacks is so painful today. Anything that allows for more easily stepping through plugins and seeing a useful backtrace is 👍 by me.

We may lose an opportunity to add behavior between each layer of the stack. I was thinking it would be nice if plugins could optionally have a name, and have next() debug-log the name, to discover which plugin was stopping the stack or whether plugins were running in the wrong order. If functions are calling other functions directly, it keeps us from adding between-plugin hooks. This is solvable later without an API change, though.

Plugins being able to use return values from other plugins would be awesome! Right now, running your code before the rest of the stack is easy, just use args. Doing interesting work around / after next is much harder, since the only way you can communicate is through the Editor or Value. This is something I've been wanting, so I'm very happy to see it being considered. I'm excited for what that would open up.

👎 on removing register*. I love that commands and queries are called right on editor, having to use exec for everything would be an API downgrade, in my opinion. I also like the flexibility of configuring it with a function, we do something similar for configuring toolbar buttons and plugins in our Slate-based editor. Configuring should be optional, because I would assume a lot of plugins won't care. It would be nice to have a "do the right thing" default, but I haven't thought about how that would work.

I'm assuming we'd keep an onExec, right? onCommand / onQuery are amazing, I'd hate to lose them!

Right now, commands and queries aren't passed next. That means if you want to compose them, you have to override onCommand. That feels weird, when all you really want to do is decorate the default behavior. It seems like this would solve that problem?

Naming: what about call instead of exec?

Overall, I like this a lot. It makes things feel a lot more like "just writing code" instead of plugging objects into a framework.

[ianstormtaylor]

@justinweiss thank you! That is very helpful for my thinking. Going through your points...

Debugging plugin stacks is so painful today. Anything that allows for more easily stepping through plugins and seeing a useful backtrace is 👍 by me.

We may lose an opportunity to add behavior between each layer of the stack. I was thinking it would be nice if plugins could optionally have a name, and have next() debug-log the name, to discover which plugin was stopping the stack or whether plugins were running in the wrong order. If functions are calling other functions directly, it keeps us from adding between-plugin hooks. This is solvable later without an API change, though.

This is something I ran into immediately while trying this out 😄 since we had previously interleaved normalizeDirtyPaths() between commands. Without a small layer in between I'm not sure how we'd solve this?

I had thought we'd be able to make it just calling functions directly, which would be glorious to step through. Not sure how to do that? Do you have any ideas?

Plugins being able to use return values from other plugins would be awesome! Right now, running your code before the rest of the stack is easy, just use args. Doing interesting work around / after next is much harder, since the only way you can communicate is through the Editor or Value. This is something I've been wanting, so I'm very happy to see it being considered. I'm excited for what that would open up.

Not exactly sure what you mean here?

👎 on removing register*. I love that commands and queries are called right on editor, having to use exec for everything would be an API downgrade, in my opinion. I also like the flexibility of configuring it with a function, we do something similar for configuring toolbar buttons and plugins in our Slate-based editor. Configuring should be optional, because I would assume a lot of plugins won't care. It would be nice to have a "do the right thing" default, but I haven't thought about how that would work.

You'd still be able to run all commands as editor.insertText directly with the new system. The registering logic being exposed was some edge case I can't recall. Does that solve your worry there?

I'm assuming we'd keep an onExec, right? onCommand / onQuery are amazing, I'd hate to lose them!

Right now, commands and queries aren't passed next. That means if you want to compose them, you have to override onCommand. That feels weird, when all you really want to do is decorate the default behavior. It seems like this would solve that problem?

Commands/queries in the new system would all receive the "old" function they are composing (essentially next) so yup. Does that eliminate your worry about having onExec? I wasn't planning on keeping it, since I've only ever used it for overriding commands, which is now built in.

Naming: what about call instead of exec?

Open to it! I think it used to be called that. Only thing I'm thinking is paralleling the DOM with execCommand naming. Small pros/cons either way, so we can revisit this in the future if we decide call is better.

Thanks!

[justinweiss]

Without a small layer in between I'm not sure how we'd solve this?

Yeah. What I meant was, it's something we could solve later by wrapping fn(). You'd have a wrapper layer in between each plugin layer, but no client code would have to change, and the backtrace would still be pretty readable -- at least, way more readable than it is today. Sounds like we need that immediately, though :-)

Not exactly sure what you mean here?

Not really important, just agreeing with you that using return values from plugin functions will be flexible. Although it's a shame we lose chaining because of it! Don't think it's possible to have both, though. And it's not like I would have missed chaining if it hadn't existed to begin with.

You'd still be able to run all commands as editor.insertText directly with the new system. The registering logic being exposed was some edge case I can't recall. Does that solve your worry there?

Yep, I think so.

Commands/queries in the new system would all receive the "old" function they are composing (essentially next) so yup. Does that eliminate your worry about having onExec? I wasn't planning on keeping it, since I've only ever used it for overriding commands, which is now built in.

That solves all of the places I'm using onCommand / onQuery for right now. I still think it would be nice to be able to intercept all commands in a plugin. Since I can't think of a specific case where I'd actually do it, it's probably not necessary.

Open to it! I think it used to be called that. Only thing I'm thinking is paralleling the DOM with execCommand naming. Small pros/cons either way, so we can revisit this in the future if we decide call is better.

Cool. Mirroring some existing thing was what I was going for, I couldn't think of what exec was meant to mirror.

Oh, something that's not clear to me yet -- are you planning to have exec work with anonymous functions, the way command does today? What would their signature look like?

[ianstormtaylor]

Nice, that's all great to hear!

Oh, something that's not clear to me yet -- are you planning to have exec work with anonymous functions, the way command does today? What would their signature look like?

I was thinking of dropping it. Do you have a use case in mind? I'm open to it. It's just that it seems weird to pass different arguments into them than normal, but also they'd be useless without getting editor. I think it's probably better for people to just call fn(editor, ...args) themselves at that point, and avoid canonicalizing it in core.

[justinweiss]

Yeah, I think it's just that I'm used to it, and have a lot of functions that are using that style. I suppose it would be easy enough to emulate in plugin-land if I needed it:

{
  command: (fn, editor) => (commandFn, ...args) => {
    commandFn(editor, ...args);
    return editor;
  }
}

editor.command((editor, arg1, arg2) => {...})

[wmertens]

Since you'd be removing some semantic context, I propose that the fn is indeed wrapped and some inspection of results is done to make sure that the plugins behave as expected, returning correct values.

Webpack has a library tapable (but it's not really browser-safe) that allows registering plugins by first registering hooks and then letting anybody attach to those hooks with a name and a function. The hooks can then be called using a variety of patterns, sync or async. I'd love to see a browser-safe tapable lookalike used in Slate; it restores some semantics and it even allows plugins to have plugins (just register a new hook).

[ianstormtaylor]

@wmertens can you explain more what your thinking? Ideally with code samples. It’s hard to understand what the core takeaways should be from Tapable, since the examples in it’s readme seem pretty convoluted.

[wmertens]

Suppose we put a hooks object on editor. Each hook will be called at some point in time. For example, a renderEditor hook.

    // ... inside the slate-react construction
    // When you register a hook you can name parameters
    // SyncWaterfallHook calls registered taps in order and passes the previous return value as the first argument
    editor.hooks.renderEditor = new SyncWaterfallHook(['app', 'editor'])

    // register old-style plugins
    options.plugins.forEach(plugin => {
        if (plugin.renderEditor) editor.hooks.tap(plugin.name, plugin.renderEditor)
    })

    // ... inside the render function
    const app = editor.hooks.renderEditor.call(false, editor)

Now, let's assume we have a plugin that does async grammar handling, and it allows the results to be post-processed. It could do:

    // ... somewhere appropriate after standard hooks are defined
    options.plugins.forEach(plugin => {
        if (plugin.init) plugin.init(editor, options) // whatever
    })

    // ... in the grammar plugin init - note no access to editor given
    // a sub-plugin would then editor.hooks.onGrammar.tapAsync(name, fn)
    editor.hooks.onGrammar = new AsyncSeriesWaterfallHook(['grammar'])

    // ... in the plugin on grammar result
    await editor.hooks.onGrammar.callAsync(grammar)

There are also ways to debug the hooks or augment them.

This gives you an idea of the concepts in tapable. So in fact it goes in the opposite direction of what you are proposing, each tap gets specialized arguments if they are useful.

The reason tapable is not browser safe is because it makes the hooks as generated scripts in ES6, and IE11 does not like that.

[ianstormtaylor]

@wmertens thanks for the detail!

I'm not sure we gain that much from it that we don't already have? Since we don't really need async-ness for any hooks. And I think we can get away with all of the hooks having the same waterfall approach by using composition instead. It seems like it pays off to avoid the extra complexity by just composing?

[ianstormtaylor]

Fixed by https://github.com/ianstormtaylor/slate/pull/3093.