KonnorRogers / rhino-editor

A tiptap integration for Rails compatible with ActionText
https://rhino-editor.vercel.app
MIT License
226 stars 11 forks source link

[BUG]: `addExtensions([])` does not add extensions when used with an array. #212

Open bjhess opened 6 days ago

bjhess commented 6 days ago

I'm not very comfortable filing this as an issue because I don't have a lot of information to provide. And I find the dependency tree for these things maddening and there's a > 50% chance this issue has nothing to do with Rhino. But I thought I'd file the issue so I can start understanding where the borders and boundaries are, as well as understanding @KonnorRogers's preferences on these things.

Right now we have the Tiptap TextAlign extension deployed successfully with:

When updating to the latest rhino-editor via bin/importmap pin "rhino-editor", this puts us more like:

Our editor button looks something like this (note the click->rhino#alignRight action):

<button slot="after-increase-indentation-button" class="rhino-toolbar-button toolbar__button--right-align" type="button" aria-describedby="right" data-action="click->rhino#alignRight" data-align="right" data-role="toolbar-item" tabindex="0">
  <role-tooltip id="right" hoist="" part="toolbar__tooltip toolbar__tooltip--bold" exportparts="base:toolbar__tooltip__base, arrow:toolbar__tooltip__arrow" role="tooltip" inert="" placement="top">
    Right Align &lt;cmd+shift+r&gt;
  </role-tooltip>
  <svg xmlns="http://www.w3.org/2000/svg" aria-hidden="true" fill="currentColor" part="toolbar__icon" viewBox="0 0 448 512" width="24" height="18" style="pointer-events: none;">
    <path d="M424 40c13.3 0 24 10.7 24 24s-10.7 24-24 24L184 88c-13.3 0-24-10.7-24-24s10.7-24 24-24l240 0zm0 128c13.3 0 24 10.7 24 24s-10.7 24-24 24L24 216c-13.3 0-24-10.7-24-24s10.7-24 24-24l400 0zm24 152c0 13.3-10.7 24-24 24l-240 0c-13.3 0-24-10.7-24-24s10.7-24 24-24l240 0c13.3 0 24 10.7 24 24zM424 424c13.3 0 24 10.7 24 24s-10.7 24-24 24L24 472c-13.3 0-24-10.7-24-24s10.7-24 24-24l400 0z"/>
  </svg>
</button>

Our implementation of alignRight looks as so in our rhino_controller.js stimulus file:

alignRight() {
  this.element.editor.chain().focus().setTextAlign('right').run()
  this.updateAlignmentButtons()
}

updateAlignmentButtons() {
  const editor = this.element.editor
  const alignments = ['left', 'center', 'right']
  alignments.forEach(alignment => {
    const button = this.element.querySelector(`rhino-editor button[data-align="${alignment}"]`)
    if (button) {
      button.classList.toggle('toolbar__button--active', editor.isActive({ textAlign: alignment }))
    }
  })
}

And our error:

Uncaught TypeError: this.element.editor.chain(...).focus(...).setTextAlign is not a function
    at eval (eval at alignRight (rhino_controller-ccad8220309dbc9f912ffa0eaf6d3ef2f5ca26f4dbae5ab3cc364cf646b23624.js:1:1), <anonymous>:1:37)
    at extended.alignRight (rhino_controller-ccad8220309dbc9f912ffa0eaf6d3ef2f5ca26f4dbae5ab3cc364cf646b23624.js:250:5)
    at Binding.invokeWithEvent (stimulus.js:385:25)
    at Binding.handleEvent (stimulus.js:350:18)
    at EventListener.handleEvent (stimulus.js:31:25)
KonnorRogers commented 6 days ago

🤔 it looks like your alignment isn't being added to the editor. Could you show me thats setting up the editor? A minimal reproduction would be super helpful if possible, otherwise I'm largely just guessing.

As for the dependency tree, I dont love it either. I even relaxed versioning in the latest release back to "^2.2.0" to allow for better deduplication, but its largely a product of the "every extension is a package" approach of TipTap, and ProseMirror itself being like 5+ packages.

Nothing about this "sparks joy"

https://generator.jspm.io/#U2NhYGBkDM0rySzJSU1hKMrIzMvXTU3JLMkvcjDQMzTSMwAAEiVj2iIA

However, it is hard to beat the ecosystem of ProseMirror and TipTap, so it is a tradeoff.

There is a pre-bundled /exports/bundle/index.module.js for CDN usage, but could hit some weird edge cases about duplicate dependencies because dependencies are "inlined" in that pre-bundled version, so I try not to have people use it unless they really need to.

https://rhino-editor.vercel.app/tutorials/getting-started#manual-importmaps-installation

bjhess commented 6 days ago

Sorry, yes, we add it as a part of our initialization process via:

    rhino.addExtensions([
      Heading,
      Typography,
      PastedMarkdown,
      Youtube,
      TextAlign.configure({
        types: ['heading', 'paragraph'],
        alignments: ['left', 'center', 'right']
      })
    ])

A minimal reproduction would be super helpful if possible, otherwise I'm largely just guessing.

I'm hoping the best way is for me to figure out how to do that via the base library Rails test app, and then maybe push up a branch with the breakage? Alternatively I can maybe put together a little Rails app with the minimum necessary to recreate? I'm happy to set up whatever your preference would be long-term for bug troubleshooting. (Either one may take me a minute, though!)

KonnorRogers commented 6 days ago

Sorry, yes, we add it as a part of our initialization process via:

    rhino.addExtensions([
      Heading,
      Typography,
      PastedMarkdown,
      Youtube,
      TextAlign.configure({
        types: ['heading', 'paragraph'],
        alignments: ['left', 'center', 'right']
      })
    ])

A minimal reproduction would be super helpful if possible, otherwise I'm largely just guessing.

I'm hoping the best way is for me to figure out how to do that via the base library Rails test app, and then maybe push up a branch with the breakage? Alternatively I can maybe put together a little Rails app with the minimum necessary to recreate? I'm happy to set up whatever your preference would be long-term for bug troubleshooting. (Either one may take me a minute, though!)

That works for me, but because you are using importmaps, it may be different since the test rails app is using Vite. I'm hoping to start putting sandboxes together to make things easier to clone down and work with.

bjhess commented 6 days ago

I tried my best to get the Rhino repo in line with what we have, and alignment didn't break so it seems unlikely that Rhino is the culprit. Here's what it looked like: https://github.com/bjhess/rhino-editor/commit/e0c95d2dcfd22218d6d056f4638d3c4013e6d351

KonnorRogers commented 6 days ago

So I tested with JSPM importmaps and it seems to be working fine.

https://codepen.io/paramagicdev/pen/poMpgEY

However, I did notice a double load issue.

KonnorRogers commented 6 days ago

Confirmed bug with arrays:

https://github.com/KonnorRogers/rhino-editor/blob/9d261e173b945a06d62d33b3c51b63bbe80763be/src/exports/elements/tip-tap-editor-base.ts#L484

-        ary.push(ext.flat(1) as unknown as AnyExtension);
+        ary = ary.concat(ext.flat(1) as unknown as AnyExtension);

Should fix it. I'll work on a regression test + fix the double extension loading issue.

bjhess commented 6 days ago

Thank you!