Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TypeScript types for Suggestion command, allowing for generic override #4136

Merged

Conversation

sjdemartini
Copy link
Contributor

Please describe your changes

As of #2610 (7cae967), 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):


attrs?: Record<string, any>

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

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

Related issues

n/a

@netlify
Copy link

netlify bot commented Jun 16, 2023

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit 9545417
🔍 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.

Copy link
Contributor

@bdbch bdbch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - @svenadlung you want to take another look?

@sjdemartini
Copy link
Contributor Author

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"].)

@devth
Copy link

devth commented Apr 5, 2024

Would love to see this merged!

@DhenPadilla
Copy link

Would also love this in!

nperez0111
nperez0111 previously approved these changes May 13, 2024
Copy link
Contributor

@nperez0111 nperez0111 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me too

bdbch
bdbch previously approved these changes May 13, 2024
Copy link
Contributor

@bdbch bdbch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bdbch
Copy link
Contributor

bdbch commented May 13, 2024

@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?

@sjdemartini sjdemartini changed the base branch from develop to main May 16, 2024 23:30
@sjdemartini sjdemartini dismissed stale reviews from bdbch and nperez0111 May 16, 2024 23:30

The base branch was changed.

As of
ueberdosis@7cae967,
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 all
`string`s, but the selected Mention is of type `{id: string}` (which are
the attributes of the Mention node, as the Mention extension requires):

```ts
  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. When using the suggestion plugin with 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 shape for suggestion
options, as in the Tiptap example above):
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

This fixes the typing so that suggestions can correctly refer separately
to their own items (of any type), while ensuring the `command`ed item be
of whatever type is necessary (and so in the Mention context, could be
restricted further).
@sjdemartini sjdemartini force-pushed the fix-typing-for-suggestion-command branch from a258e38 to 9545417 Compare May 17, 2024 00:23
@sjdemartini
Copy link
Contributor Author

@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.

Copy link
Contributor

@nperez0111 nperez0111 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to merge

@nperez0111 nperez0111 merged commit f55171f into ueberdosis:main May 17, 2024
14 checks passed
Dalcvi pushed a commit to Dalcvi/tiptap that referenced this pull request May 28, 2024
…rdosis#4136)

* Fix typing for Suggestion `command` with new MentionAttrs generic

As of
ueberdosis@7cae967,
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 all
`string`s, but the selected Mention is of type `{id: string}` (which are
the attributes of the Mention node, as the Mention extension requires):

```ts
  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. When using the suggestion plugin with 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 shape for suggestion
options, as in the Tiptap example above):
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

This fixes the typing so that suggestions can correctly refer separately
to their own items (of any type), while ensuring the `command`ed item be
of whatever type is necessary (and so in the Mention context, could be
restricted further).

* Add generics to override selected suggestion type

---------

Co-authored-by: Steven DeMartini <[email protected]>
@sjdemartini sjdemartini deleted the fix-typing-for-suggestion-command branch July 19, 2024 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants