Slate: Add an `editor` prop to the `Editor` react component.

[brendancarney]

Do you want to request a _feature_ or report a _bug_?

Feature

What's the expected behavior?

Currently, you can only get the editor controller instance by using refs. I think being able to instantiate your Editor controller above your component in the tree, and pass it as a prop to the editor would improve the ergonomics a bit when you need to interact with your editor outside of plugins. This can also sort of be accomplished with renderEditor, but at the expense of a bit of clarity.

Example:

function App() {
  const editor = new SlateEditor({plugins})

  return (
    <Editor
         editor={editor} // could be `controller` maybe?
    />
  )
}

If this were implemented, we could remove ref support and cleanup the editor component code a bit. You could still technically do the same thing, you'd just have grab the editor from the props of your ref vs acting on your ref directly.

Thoughts?

[alanchrt]

Couldn't agree more. I've had multiple instances where I wished I didn't have to re-instantiate a separate editor controller outside the context of my React component hierarchy. This would be really useful for lots of scenarios (including tests).

[ianstormtaylor]

For testing, you can use the non-React editor controller that is exported from Slate core.

I’m not sure this buys us anything other than not having to use refs for sibling components. But I’m not sure that is a big enough gain. Seeing as you’d have to use refs already in normal React to do similar.

[alanchrt]

@ianstormtaylor I do exactly that for testing, but it's still awkward to have to instantiate two editors (or wrap with a React component to get the same instance) if you need to pass the editor to another component or use it in an external context.

As an example, here's a test where I have a Toolbar that takes the editor controller as a prop. I need to render the editor into the DOM and check that on mouse click when the first line has the selection (in editor value), the popper target for the Toolbar is the first line. I was able to hack around it by passing the editor value (instead of the controller itself) into my Slate editor so at least the keys matched, but they aren't technically the same editor instance:

test('sets the target to the DOM node of the first selected block', () => {
  const div = document.createElement('div');
  document.body.appendChild(div);

  const editor = makeEditor({ value: multiselected });  // instantiates an editor controller with lines selected
  const editorElement = renderEditor({ value: editor.value });  // renders a Slate Editor with the controller's value
  const wrapper = mount((
    <div>
      <Toolbar editor={editor} />
      {editorElement}
    </div>
  ), { attachTo: div });

  mouseClick$.next(new MouseEvent('click'));

  const menu = wrapper.update().find(ToolbarMenu);
  const targetText = menu.prop('popperTarget').textContent;
  const nodeText = multiselected.document.nodes.first().text;
  expect(targetText).toEqual(nodeText);  // hacky assertion to get around some DOM comparison weirdness
});

Now, if I had actually needed the editor to be the same instance of the controller (for example, if I was testing an editor command had a certain effect), I would need to wrap with a component to get at the ref and pass into my Toolbar:

test('sets the target to the DOM node of the first selected block', () => {
  const div = document.createElement('div');
  document.body.appendChild(div);

  class Wrapper extends Component {
    constructor() {
      this.editor = React.createRef();
    }

    render() {
      return (
        <div>
          <Toolbar editor={this.editor} />
          <Editor
            value={multiselected}
            ref={this.editor}
          />
        </div>
      );
    }
  }

  const wrapper = mount(Wrapper, { attachTo: div });

  mouseClick$.next(new MouseEvent('click'));

  const menu = wrapper.update().find(ToolbarMenu);
  const targetText = menu.prop('popperTarget').textContent;
  const nodeText = multiselected.document.nodes.first().text;
  expect(targetText).toEqual(nodeText);
});

This is just cumbersome boilerplate. If we had what @brendancarney is suggesting, it could be as simple as the original test, but using the _actual_ editor controller instance, not just the value.

test('sets the target to the DOM node of the first selected block', () => {
  const div = document.createElement('div');
  document.body.appendChild(div);

  const editor = makeEditor({ value: multiselected });  // instantiates an editor controller with lines selected
  const editorElement = renderEditor({ editor });  // renders a Slate Editor with actual editor controller
  const wrapper = mount((
    <div>
      <Toolbar editor={editor} />
      {editorElement}
    </div>
  ), { attachTo: div });

  mouseClick$.next(new MouseEvent('click'));

  const menu = wrapper.update().find(ToolbarMenu);
  const targetText = menu.prop('popperTarget').textContent;
  const nodeText = multiselected.document.nodes.first().text;
  expect(targetText).toEqual(nodeText);
});

Maybe there's a better way to do this overall, but I've run into several cases, both in my code base and in my tests, where it simplifies getting at the actual editor controller to be able to instantiate it externally to the React Editor view layer.

[brendancarney]

@ianstormtaylor Admittedly, this may mostly be personal preference. I do think that allowing the passing in of the editor or a non-ref API slightly improves clarity. Here are a couple reasons:

• When you build controls that are above the editor in the tree, it feels awkward to grab the ref from the component to have to do that. There aren't many other cases where I use a ref to call methods on a component instance (focus() is the only one I can think of).
• It doesn't require rendering the editor to obtain a reference to it. While maybe that hasn't been a huge issue in practice, it just _feels_ odd. (I know that's not a great reason 🙂)

An alternative implementation could be an optional wrapper component for the editor that allows access to the editor via a hook. Example:

App:

import { Editor, EditorContext } from "slate-react" 
const App = () => (
   <EditorContext>
      <Toolbar/>
      <Editor/>
   </EditorContext>  
)

Toolbar:

const Toolbar = () => {
  const editor = useEditor();

  ...
}

I completely understand if you don't think it's worth it at this time. But, if you want to see more before deciding, I'd be happy to put together a quick draft PR. Thanks!

[ianstormtaylor]

Related to https://github.com/ianstormtaylor/slate/issues/2668, I think the unresearched part of this issue is what the cons are to making this change.

We currently attach extra properties to editor in React-land, which means that we'd need to find another way to handle those properties if we don't let the <Editor> instantiate its own editor. I haven't looked into what the alternative is—it could be nicer for all I know—but that would need to be figured out to properly evaluate this issue I think.

[brendancarney]

I've been thinking about this more and doing some experimenting in our code base. I do think we should work away from the ref approach for a couple reasons:

• Function components can't have refs, and while react will support classes for a long time, it's clear the future is all components being functions, and there are at least a few bugs that I know could be solved cleanly with hooks in the editor component.
• It's not very ergonomic. I think we can come up with a better way that will be less confusing for new users.

I'm not sure if this is best, but here's what I'm thinking so far:

Basic Usage

import { useSlate, Editor } from "slate-react"

function App() {
  const editor = useSlate({ plugins: [], ...otherOptions}) 

  return <Editor editor={editor} />
}

This way, editor always exists vs in the ref approach where Editor has to render first.

[ianstormtaylor]

Sounds brilliant to me!

[ianstormtaylor]

Actually thinking about this more, and I’m still not sure the hook version works. Almost all the props currently passed into <Editor> would need to be passed into the hook instead, since they are plugin-level configuration. Which ends up feeling awkward.

Not only that, but you’ll still need the ref in cases where you want to retrieve the DOM element of the editor.

All of this is to avoid having to render toolbar-like components underneath the editor component, which isn’t really cumbersome at all.

If you change your mindset to think of <Editor> as a context-like provider, then having to be underneath it to retrieve it makes total sense.

This seems like a case where renderEditor= should be used to render the toolbar in the context of the editor itself, instead of awkwardly passing things between sibling components.

[brendancarney]

If you change your mindset to think of as a context-like provider, then having to be underneath it to retrieve it makes total sense.

From my previous comments, you'll see this is what I'm going for, and I think renderEditor _is_ cumbersome. It's maybe not too bad for some contrived example with one toolbar, but for an app with a lot of controls outside the editor's content area, renderEditor is awkward.

I'd much rather have this structure:

function App() {
  return (
    <div>
      <MainContentArea>
        <Editor />
        <Toolbar editor={editor} someProp={fromAppState} />
      </MainContentArea>

      <Sidebar>
        <OtherControls editor={editor}/>
      </Sidebar>
    </div>
  );
}

than have to do all this with renderEditor or a ref.

Almost all the props currently passed into would need to be passed into the hook instead, since they are plugin-level configuration. Which ends up feeling awkward.

Maybe I don't follow, but I don't see how this is awkward. I think some separation of what is passed to the editor and the component could be a good thing. The only one that maybe feels awkward to me is renderEditor, but I think the use of that would go way down in this setup.

[ianstormtaylor]

@brendancarney but is that more cumbersome than what would be involved with having to use a hook and pass in a bunch of arguments to it, just to be able to pass that hook around? For example here's what that would look like with renderEditor:

const MyEditor = props => {
  return (
    <Editor
      renderEditor={(editor, next) => (
        <div>
          <MainContentArea>
            {next()}
            <Toolbar editor={editor} someProp={fromAppState} />
          </MainContentArea>
          <Sidebar>
            <OtherControls editor={editor}/>
          </Sidebar>
        </div>
      )}
    />
  )
}  

And the benefit to that is that now <MyEditor> is self contained and easier to test. (Most likely you already have some sort of wrapping component like this already, instead of nesting straight into <App>.)

Potentially the renderEditor name is what trips people up. If it was like renderContent instead it might feel less strange.

[brendancarney]

Yeah, maybe some of it is just personal preference. I don't understand why it would be more involved with the hook approach though. You have to pass the arguments somewhere. Maybe a slight variation would simplify it:

const MyEditor = props => {
  const [Editor, controller] = useSlate({ plugins: [], ...otherSlateArgs })

  return (
    <div>
      <MainContent>
        <Editor {...DOMSpecificProps} data-some-attr="" />
      </MainContent>
    </div>
  )
}

In either case, I think accessing the editor controller via a ref is the awkward part of this whole thing.
If everything is possible via renderEditor, or a hooks based approach, I think we should deprecate accessing the editor via ref, and update ref to give you the actual editor DOM node.

Anyways, I appreciate you taking the time to discuss this. I'll experiment with it a bit more in our codebase.

[ianstormtaylor]

@brendancarney I'm not totally opposed! I just want to avoid having to solve these things with two separate concepts, and I think I'm hesitant because I see some issues to solve that I'm unsure how to solve. But maybe we just need to figure out how to solve them and the hooks solution would be a better one.

Right now there are two groups of props that <Editor> takes:

1. View-layer-specific props like autoCorrect, autoFocus, etc. These are mostly passed straight through to the contenteditable DOM node, but some are also used by the lower-level components in the view layer.

2. Controller-specific props like plugins, schema, onKeyDown, renderNode, etc. Anything that can be defined in a plugin. Whenever these change a new controller has to be created.

All of the controller-specific props would go into the hook, so you'd end up with something like:

const MyEditor = props => {
  const editor = useEditor({
    plugins: [...],
    onKeyDown: (event, editor, next) => { ... },
    renderBlock: (props, editor, next) => { ... },
  })

  return (
    <div>
      <Editor
        editor={editor}
        autoCorrect={false}
        placeholder="..."
        ...
      />
      ...
    </div>
  )
}

Off the top of my head, I see a few issues to work through with this:

• The onKeyDown, renderNode, etc. top-level helpers become awkward, since they feel like React-like handlers but are passed into a hook. I'd probably opt to eliminate them, and just force everything to be defined in a plugin at this point, so that the hook API becomes useEditor(plugins). (This could be a nice thing actually? But it means removing the current hassle-free way of defining things.)

• I think the plugins: [...] value there would need to be memoized? Because we can't re-render each time the plugins change. Although this is already an issue, so nevermind. I need to figure out how memoization works with React hooks—it seems like this is people's biggest issue with them.

• I'm unsure of how this will work with value. Right now the controller takes in the value from props, but it's actually a controller-level argument. Potentially we'd need to change the hook to be useEditor(value, plugins) instead? I'm not quite sure how this works with memoization.

• And the related-ly, how would it work with defaultValue? I'd like to preserve the ability for the editor to be either controller or uncontrolled.

• I'd like to introduce a useEditor() hook later for accessing the editor controller itself at any level of the React tree. So we'll probably need to find a different name for the top-level one?

I haven't fully investigated React hooks, so I'm unsure of whether some of these are surface-level and could be solved better.

But this would mean, we'd end up with something more like:

const MyEditor = props => {
  const config = useMemo(() => {
    return [
      {
        onKeyDown: (event, editor, next) => {},
        renderBlock: (props, editor, next) => {},
      },
      ...
    ]
  }, [...])

  const plugins = usePlugins(config)

  return (
    <div>
      <Editor
        plugins={plugins}
        autoCorrect={false}
        value={value}
        placeholder="..."
        ...
      />
      ...
    </div>
  )
}

The biggest thing we lose is the easy passing in of things like renderBlock. But I'm okay to lose those if necessary. I'm just unsure if there are things I'm overlooking where view-layer logic is actually controller-layer logic.

We also gain simplicity in the internals because we could probably combine <Editor> and <Content> at that point.

---

In the further future, a simple example case might look something like...

const plugins = [{
  onKeyDown: (fn, editor) => event => { ... },
  renderBlock: (fn, editor) => props => { ... },
  ...
}]

const MyEditor = props => {
  const adapter = useAdapter(plugins)
  return (
    <Editor
      adapter={adapter}
      autoCorrect={false}
      value={value}
      placeholder="..."
      ...
    />
  )
}

...which does have niceness to it.

I'm down to try moving in this direction. I think someone would just need to spearhead it. I think there are some gotchas that aren't obvious up front that would need to be handled well. I just don't want to end up with having two separate concepts that confuse people as to where the boundaries are.

[brendancarney]

A few thoughts in response:

View-layer-specific props
I think there are two options here. One is that these are defined in the hook, and the hook also gives you a component:

const [Editor, controller] = useSlate({ plugins, spellCheck: false})

The other is that anything that gets passed directly to the DOM node is passed to the component. I think it should behave this way either way, so it makes sense to support it.

Value
Yes, I think you'd pass value to the hook also. For default value, we could still support that with state inside the hook.

I'd like to introduce a useEditor() hook later for accessing the editor

I do exactly this in our application. I love it. I'd recommend useSlate as the main hook.

---

I'm down to put some time into working out a proof of concept whether we go this way or not. The biggest thing for me is getting ConvertKit's editors up to date with the latest versions first, so it might be a little bit before I get started.

[ianstormtaylor]

I believe that this may be fixed by https://github.com/ianstormtaylor/slate/pull/3093, which has changed a lot of the logic in Slate and slate-react especially. I'm going to close this out, but as always, feel free to open a new issue if it persists for you. Thanks for understanding.