lxsmnsyc / solid-tiptap

SolidJS bindings for tiptap
MIT License
114 stars 3 forks source link

[Question] Is `createEditorTransaction` really needed? #8

Closed amirhhashemi closed 1 year ago

amirhhashemi commented 1 year ago

I don't like createEditorTransaction. It just subscribes to the "transaction" event and re-runs its second argument whenever a transaction happens. I guess the reason is that the editor has a non-reactive internal state and the only way we know its state has changed is when the "transaction" event fires. So to run a command on the latest version of the editor we must subscribe to the "transaction" event and get the latest instance every time it fires and then run the command.

In a realistic project it would probably be used over 20 times and that means we are subscribing to the "transaction" event 20 times and 20 signals (the forceUpdate signal) will be changed for every transaction and then they cause another signal (the editor) to be called again to get the new editor and then we can run the editor command. I haven't tested it on a large project yet but it doesn't sound good. Also, the syntax is very verbose.

Another problem is that running commands that change the editor state doesn't work! For example when I run this:

createEditorTransaction(
  () => props.editor,
  (e) => e.chain().focus().toggleBold().run(),
);

I only get a warning on the console:

dev.js:753 computations created outside a `createRoot` or `render` will never be disposed

And nothing happens. I'm not sure why.

Another approach is to make the editor instance that createTiptapEditor returns reactive (re-get the new editor on every transaction). That way we would only need to subscribe to "transaction" only once and we wouldn't need createEditorTransaction, we can run commands directly on editor:

export default function createTiptapEditor<T extends HTMLElement>(
  props: () => UseEditorOptions<T>,
): () => Editor | undefined {
  const [editor, setEditor] = createSignal<Editor>();
  const [depend, rerun] = createSignal(undefined, { equals: false });

  createEffect(() => {
    const instance = new Editor(props());

    instance.on("transaction", rerun);
    setEditor(instance);

    onCleanup(() => {
      instance.destroy();
    });
  });

  return () => {
    depend();
    return editor();
  };
}

const Component = () => {
  const editor = createTiptapEditor(() => {})
  return (
    <Show when={editor()?.isActive("bold")}>
      bold
    </Show>
  )
}

That's closer to the React implementation. It works fine in my small tests but I'm wondering what your thoughts are? I'm very new Solid and I'm still learning it.

lxsmnsyc commented 1 year ago

I don't think it's a good idea because people would think that the properties itself are reactive but mainly because they are tracking the editor instance. If one was to wrap the editor in createMemo (a common practice when using the Show component), this whole idea just fails this is why it is important to just rely on subscribing to transactions. Aside from the fact that this kills fine-grained updates, using a single transaction subscribers vs N transaction subscribers wouldn't really make a difference when transposed into a signal, you'd still be making the same number of execution.