-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fix TypeScript types for Suggestion command
, allowing for generic override
#4136
Conversation
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this 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?
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 |
Would love to see this merged! |
Would also love this in! |
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@sjdemartini we have a few issues with the branches. Could you
|
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).
a258e38
to
9545417
Compare
@bdbch and @nperez0111 Thanks for following up on this! I've changed the target branch to Let me know if there's anything else needed to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to merge
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 theitems
in the suggestion options. For instance, in Tiptap's official demo https://tiptap.dev/api/nodes/mention, the suggestionitems
are eachstring
s, but the selected Mention is of type{id: string;}
(for the attributes of the Mention node):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 SuggestionPropsprops.command()
function must be aRecord<string, any>
, as it's directly/exclusively used to set theattrs
of aNode
viainsertContentAt
(and you need not use that identical shape for suggestion options themselves):tiptap/packages/extension-mention/src/mention.ts
Line 42 in 44996d6
tiptap/packages/core/src/types.ts
Line 79 in f869507
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 thecommand
ed 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, theMention
extension now has more specific typing so that it requires a configuredsuggestion
to pass in objects of the expected structure (id
andlabel
).How have you tested your changes
Tested these types with my local version of suggestion/mention extensions, included an extension that
extend
sMention
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 toany
(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
Related issues
n/a