ianstormtaylor / slate

A completely customizable framework for building rich text editors. (Currently in beta.)
http://slatejs.org
MIT License
29.81k stars 3.24k forks source link

eventually decide on how to handle commands #2342

Closed ianstormtaylor closed 5 years ago

ianstormtaylor commented 5 years ago

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

Discussion.

What's the current behavior?

Right now we have a combination of three different ways for commands to be defined and executed, for a variety of reasons, and it feels like we might need to decide on exactly what we want, and maybe deprecate some of them...

If there were no such thing as userland commands, it would be easy to just have all the commands live as top-level methods on the editor and be done with it:

editor.insertText('word')

In this case it wouldn't ever be necessary to use a string-based syntax, because there is a fixed pool of commands.

But userland commands are very much a thing, so then we end up having to make commands definable by plugins, and shareable across plugins. To do this, right now we allow for commands to be "registered" with the editor, such that they appear at dynamic top-level methods to be used:

editor.insertLink('https://example.com')

These aren't defined in core, but in userland. Although this is great from an ergonomics point of view, it's a bit scary to make the editor's top-level namespace dynamic. An alternative, which is currently available in the API is to pass strings instead:

editor.command('insertLink', 'https://example.com')

But this has it's own awkwardness as a tradeoff. Instead, we might decide (as @Dundercover suggested recently) to pass functions around as commands, which would be tenable for both core and userland. You could define commands as functions and then just import exactly what you need when you need it:

import { insertLink } from './my-commands'

editor.command(insertLink, 'https://example.com')

This is actually also already possible with the API, since editor.command takes either a string or a function. And this is a great solution so far—passing functions is nice and easy to debug. Commands wouldn't need to be "registered", and we still get a nice syntax. Up to this point, I'd personally go with this function-passing.

However, there's one more constraint where function-passing becomes problematic... the ability to override commands from plugins. For example, say you want to tweak the behavior of the core splitBlock command when inside a code block to insert soft breaks. To do this, we can't just map commands directly to a single function as define-time, since their effects are now determined by plugins.

Which is why we have our current system, where commands are actually just shortcuts for calling through the onCommand middleware stack to be handled. But it is definitely a tradeoff in terms of debugging experience too—just as Express middleware can be hard to reason about sometimes.

To use the function-passing approach while still having the "overridability" constraint satisfied, I think we'd need to use a compose-style pattern for commands. Such that commands could be wrapped and overridden. But this also necessitates somehow getting these command definitions to plugins to do the wrapping, and then getting them to the editor to use for its internal logic. It would be different than our middleware-based approach.

So the only reason I open this issue is for people to think about this over time as they use Slate, and perhaps we might have more information to make a hard decision about which direction to go in.

Dundercover commented 5 years ago

Making commands overridable hurts more than it helps I feel. Do we know how much it is used/needed?

In the Express case you want to be able to run your own logic before/after/instead of some other code for different phases of the execution flow. I don't see that case for commands (or queries) in Slate. For example, If I want to change the behaviour of the splitblock command I would much rather create a custom mySplitBlock for the new behaviour and then just use that one instead.

If I have a bunch of third-party plugins that I cannot change splitblock of then yes, having some way of partially modifying their behaviour might be the easiest or quickest way to get what I want but I feel like it will come and bite me in the long run if I do go down that path. Making sure the commands only do what they were made to do keeps the contagion down and makes it much easier to reason about in my opinion.

Maybe I'm missing something with the overridability though, does anyone have a concrete use case for when they needed it to give me some more insight?

ianstormtaylor commented 5 years ago

Hey @Dundercover, thanks for the response! That's a good question about overridability—I agree it adds a layer of complexity.

Here's an example... consider a common behavior where if the user hits backspace at the beginning of a quote block, you want to change it back into a normal paragraph instead of merging its contents into the previous block. This can be modeled as:

function onCommand(command, editor, next) {
  const { value } = editor
  const { selection } = value
  const { isCollapsed, start } = selection

  if (
    command.type === 'deleteCharBackward' &&
    isCollapsed &&
    start.offset === 0 &&
    editor.isQuoteActive()
  ) {
    editor.unsetQuote()
    return
  }

  next()
}

Previously, we might have tried to model this with DOM-level events, listening for event.key === Backspace instead. But this presents problems because there are multiple different hotkeys that can trigger a "delete backward" command, and there's no way for a plugin to guarantee it has caught all of them. Further, with touch keyboards on mobile some user agents no longer represent keys in the traditional model (eg. Android), which is a trend I think will continue.

By allowing commands to be overridable, we encourage an architecture where events are translated into semantic commands, and all of the "behavioral" logic happens in Slate constructs. This makes it easier to adapt behaviors across environments, and easier to test the behaviors without mocking the environments as well.

Does that make sense?

I'd be open to other ways to achieve this though, if we can think of a solution that satisfies the constraints without the complexity of overriding commands.

ianstormtaylor commented 5 years ago

One of the issues that leads us to overridability is that the "commands" concept is kind of overloaded with two meanings, (a) the "intent" of the user translated from the events in the environment, and (b) the manipulations to take on the current value.

I'd be down to consider a better way to split these into two layers, which be beneficial if it means we can eliminate the need to have commands be overridable, despite the complexity of a new layer.

Dundercover commented 5 years ago

I see, thank you for clarifying. I'm uncertain of the broader 'intent' vs 'manipulation' problem but regardless of how that pans out I feel it is important that we are able to be consistent when we use this concept of commands in the form it exists today/tomorrow. So if we would pass functions as commands I don't think it is a good idea to use strings when modifying the behaviour of commands, even if it would be possible to do so using some (hackish) way. Would it be possible to do something like this instead:

import { deleteCharBackward } from 'slate/commands'

...

if (command === deleteCharBackward) { 

Or some variation of it?

ianstormtaylor commented 5 years ago

@Dundercover maybe that would be possible, but it feels a bit weird to be comparing by reference to a function. I think it might result in strange architectures especially when a plugin defines its own commands.

But... pursuing the idea of functional commands, since I agree with you that it would help to reduce complexity... we still need this concept of "intent" (or "action" to be Redux-familiar) to be able to let people intercept certain actions and remap them to new behaviors. This is pretty much the equivalent of what the beforeinput events in the DOM API tries to enable. And it is the constraint that is going to allow us to adapt to mobile keyboards where "keys" don't exist, and to other advanced use cases like intercepting browser-level input events.

If we're looking to have commands and queries end up as simple functions that get passed around and imported, they'd be something like:

import { myCommand } from './commands'
import { myQuery } from './queries'

// Invoke a command.
myCommand(editor)

// Run a query.
const result = myQuery(editor)

This is a noble goal, because it's very easy to understand. (And works well with tooling, whether its typings, bundle optimizations, whatever.)

The problem becomes that these commands aren't intercept-able. These couldn't be the top-level API that gets called, because we need to have that be a layer that plugins can tap into. That layer still needs to look (I think) like the existing "commands" layer does today.

editor.insertText(...args)

Which are still "registered" as commands are today, and become available as top-level methods on the editor instance itself. And these actions would run through the middleware, for example...

import { insertTextAtRange } from 'slate-commands'

onAction(action, editor, next) {
  if (action.type === 'insertText') {
    const { value } = editor
    const { selection } = value
    insertTextAtRange(editor, selection)
    return
  }

  next()
}

(This has a nice benefit of clearing up the confusion between "at the current range commands" and "at a specific range commands". Really the current-range ones are just the top-level actions to take in the context of the editor itself.)

This solves the commands layer. But there's still a question of the current queries that are intercept-able too... isVoid(), isAtomic(), and in the future things like getFragmentFromData(). I'm not sure how we solve these.

Edit: To be clear, the naming here is all speculative. It would probably be better to keep onAction as onCommand since we already have that concept. I'm just interested in the architecture first, then naming second.

Dundercover commented 5 years ago

I was thinking that we could still do editor.command(myCommand) but use the function reference as the identifier instead of a string. You could then intercept commands using plugins like you do today.

But yeah, you might be correct that it would be more cumbersome to use that approach when you write plugins that define their own commands. But I think it will help with maintainability since you will have explicit references everywhere (can't type the wrong command name, hit shortcut to find all references in your IDE of choice etc).

Side note: I think the best way how to view a command is an intent with a default behavior (i.e. putting the intent concept before the manipulation) since you could overwrite the manipulation if you want to.

ianstormtaylor commented 5 years ago

@Dundercover I don’t really understand how that approach would work while still letting multiple plugins override commands. Every plugin would become tightly coupled to any other plugin that overrode a command? It’s not a pattern that I’ve seen anywhere. Whereas using type strings as we currently do is very similar to Redux.

Dundercover commented 5 years ago

Yeah, if a plugin depends on three other plugins then that plugin needs to explicitly reference those other three plugins. Maybe that is too strict, decoupling everything (like Redux) has been nice, I must say, for when the codebase has grown (relatively) large.

A couple of problems I have with the comparison to redux:

dselman commented 5 years ago

Thoughts:

  1. An API on the editor to register commands (functions) by id as well as by command name. Each command would have a method getIdentifier which returns the globally unique id for this command. It could be something like org.acme.MyBoldCommand for example.

Register the function myBoldFunction against the logical command/intent bold. The command's identifier has been defined by the command developer as org.acme.MyBoldCommand:

editor.register('bold', myBoldFunction)

  1. Commands should be stateless and return a delta to be applied to the editor

  2. People chain commands by looking them up in the command registry (maintained by the editor?) and then calling them. They can then modify the return value of a command before returning it themselves.

  3. An apply/reconcile API that applies the set of commands to the editor state

This keeps commands functional, which helps with composition.

This should allow someone to import commands and drop them in to an editor, or to override existing commands, or to override existing commands by calling an existing commands and modifying its return values.

thesunny commented 5 years ago

I'm not sure about overriding core commands. I think higher level commands will depend on the behavior of core commands to do things in a specific way.

For example, somebody overrides a core command so that in an image block, splitBlock always inserts a blank block before the image block (i.e. hitting enter in an image block always inserts a blank block before). Another developer wants to add a command that adds a caption below the image. So he does a splitBlock at the end of the image block and inserts a new caption block but it appears before instead of after the image. He can't figure out what he did wrong. This is contrived but as more overrides pile up on the core command, I wonder if the number of issues would become larger and debugging becomes harder.

As mentioned earlier, there appear to be two types of commands:

I also think within custom commands, there should be two types:

"intent" commands are to me the major customization point for built-in Slate behavior. In thinking about this, I don't know if there are any more than a few intent commands that matter and maybe a few others that might be nice to have. The ones that matter are:

One thing I like about creating intent commands instead of making all the core commands available is that we are being very explicit about what we want to make overridable. This explicitness, as a user, translates to knowing that this particular customization will be a first class citizen and is designed to be supported in this manner.

Note that having an intent command solves the image block/add caption problem. We add a hook for the enter intent command so that when it's in a block, it always splits before. When we want to insert a caption, we call insertBlock which hasn't been messed with.

In order to make this all work, I think that we should make a way to add onCommand hooks to handle the behavior of commands in such a manner that it will throw an error if we override a core command or really any property that is already taken by something core (e.g. editor.props).

I will say also that despite the fact that is not as easy to differentiate command types, there is something beautiful and simple about editor.someCommand and being able to chain it that I would miss if we want to another syntax.

ianstormtaylor commented 5 years ago

@thesunny I agree with you that overrides have the potential to become confusing if people are relying on them for overlapping behaviors. But I'm not sure I see a way around that, without limiting what plugins can accomplish.

The thing is an enter command would be the same as a splitBlock. Adding an extra layer of indirection would make it possible to distinguish two of them, but it would add a lot more confusion as to which command should be overridden. You could already argue that splitBlockAtRange is the lower-level one that you can use without intent. And then enter would end up being overloaded just as much? (Not to mention that enter is named after a keyboard concept that might not be true of all cases.)

I think it's up to developers and plugins to be clear about what they do. We just give them the power to override, they use it as they want.

The questions they have to ask are...

But I think adding an extra layer there doesn't really gain you much that the others don't already give you?

ianstormtaylor commented 5 years ago

After thinking about this for a while, I think I've arrived at a good solution in https://github.com/ianstormtaylor/slate/issues/2861. Feel free to leave any comments/thoughts/problems there, thanks!