tiptap icon indicating copy to clipboard operation
tiptap copied to clipboard

Fix TypeScript types for Suggestion `command`, allowing for generic override

Open sjdemartini opened this issue 2 years ago • 4 comments

Please describe your changes

As of https://github.com/ueberdosis/tiptap/pull/2610 (https://github.com/ueberdosis/tiptap/commit/7cae9673f0086973b4d31e1343375ed5ad04ee0a), new generics were added for Suggestion options and props. However, there is a subtle bug in the current typing: the object selected with the suggestion command need not have the same types as the items in the suggestion options. For instance, in Tiptap's official demo https://tiptap.dev/api/nodes/mention, the suggestion items are each strings, but the selected Mention is of type {id: string;} (for the attributes of the Mention node):

  const selectItem = index => {
    const item = props.items[index]

    if (item) {
      props.command({ id: item })
    }
  }

i.e., there should be no restriction that when you select something with the suggestion command, it must use the identical structure as the suggested items, as that results in a type error in the above perfectly functional code. When using the suggestion plugin within the Mention extension, for instance, the value passed to the SuggestionProps props.command() function must be a Record<string, any>, as it's directly/exclusively used to set the attrs of a Node via insertContentAt (and you need not use that identical shape for suggestion options themselves):

https://github.com/ueberdosis/tiptap/blob/44996d60bebd80f3dcc897909f59d83a0eff6337/packages/extension-mention/src/mention.ts#L42 https://github.com/ueberdosis/tiptap/blob/f8695073968c5c6865ad8faf05351020abb2a3cc/packages/core/src/types.ts#L79

How did you accomplish your changes

This fixes the type definitions so that suggestions can correctly refer separately to their own items (of any type), while ensuring the commanded item can be defined with its own type definition. This allows use with the Mention extension, where you can still require the correct object structure needed for mention node attributes. In addition, the Mention extension now has more specific typing so that it requires a configured suggestion to pass in objects of the expected structure (id and label).

How have you tested your changes

Tested these types with my local version of suggestion/mention extensions, included an extension that extends Mention and further overrides its attributes, and it resolved type errors that were otherwise present.

How can we verify your changes

Use TypeScript with the original mention example from Tiptap.

Remarks

There are two commits: the first is the simplest fix, which just sets the command argument to any (most basic way to resolve the type error). The second commit adds a generic so that the type can be specified and narrowed by callers, which the Mention extension now does by default as well.

Checklist

  • [x] The changes are not breaking the editor
  • [ ] Added tests where possible
  • [x] Followed the guidelines
  • [x] Fixed linting issues

Related issues

n/a

sjdemartini avatar Jun 16 '23 02:06 sjdemartini

Deploy Preview for tiptap-embed ready!

Name Link
Latest commit 95454177872890e246979de1da0acd737e215810
Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/6646a378d29b120008cf70c1
Deploy Preview https://deploy-preview-4136--tiptap-embed.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Jun 16 '23 02:06 netlify[bot]

Thanks for the initial review! I have a complete example here which I think is helpful to illustrate the problem and how this PR solves it. (Notice the @ts-expect-error that's necessary to silence the TS error for SuggestionProps["command"].)

sjdemartini avatar Aug 16 '23 20:08 sjdemartini

Would love to see this merged!

devth avatar Apr 05 '24 12:04 devth

Would also love this in!

DhenPadilla avatar May 09 '24 05:05 DhenPadilla

@sjdemartini we have a few issues with the branches. Could you

  1. Rebase your changes to the current main branch
  2. Change the target branch to main?

bdbch avatar May 13 '24 09:05 bdbch

@bdbch and @nperez0111 Thanks for following up on this! I've changed the target branch to main, and rebased and handled some conflicts. I tested this branch's updated version locally against https://github.com/sjdemartini/mui-tiptap (using https://github.com/wclr/yalc) and all worked well for me, and I was able to remove the @ts-expect-error in that project related to props.command() thanks to the updated types here.

Let me know if there's anything else needed to merge.

sjdemartini avatar May 17 '24 00:05 sjdemartini