Make matchers more strongly typed #4083
Conversation
| const CLIPBOARD_CONFIG: [Selector, Matcher][] = [ | ||
| type MatcherConfig = | ||
| | [TextSelector, TextMatcher] | ||
| | [ElementSelector, ElementMatcher] |
There was a problem hiding this comment.
Not sure if it's safe in ElementMatcher to consider the first parameter as Element instead of Node because TextSelector and ElementSelector both contains string as the part of the union.
There was a problem hiding this comment.
Please note: I've revised the PR types once more, but I think your concerns are addressed because:
TextSelectormaps toNode['TEXT_NODE'], soTextMatcheris only for Text nodesElementSelectormaps tostring | Node['ELEMENT_NODE'], soElementMatcheris only for Element nodes- It is possible for a matcher to meet both the
TextMatcherandElementMatchersignature at the same time (seematchNewLinefor an example). In this case, it defines itself as(node: Text | Element, ...) => ...so it can be used in either situation.
There was a problem hiding this comment.
@luin Let me know if you're still unsure about this change or whether you'd like time to consider it. I'm happy to close the PR if you think it's better in its current state.
|
|
||
| const CLIPBOARD_CONFIG: [Selector, Matcher][] = [ | ||
| type TextMatcher = (node: Text, delta: Delta, scroll: ScrollBlot) => Delta; |
There was a problem hiding this comment.
Looks like in some cases this change makes things a bit harder. E.g. https://share.slab.com/2dv3PhtG, previously, the parameters have the correct typing, but with this change, TypeScript can't infer the types and require users to provide.
There was a problem hiding this comment.
You're correct -- inference isn't really possible and that was actually intentional. The goal of the PR was that the matchers need to be strongly typed to prevent misconfiguration.
In your example, previously the parameters would have just simply been (node: Node, ...). While it is inferring the signature, it isn't protecting against mismatch.
Is type inference a requirement here? If so, then I would say the PR should be closed.
There was a problem hiding this comment.
Is type inference a requirement here?
Yeah I think it is. One reason is that we aim to minimize the effort required from users when upgrading to version 2.0.
There was a problem hiding this comment.
Okay no problem, then probably this PR would do more harm than good so I'll close it out!
This adds stronger type declarations for the Matchers and Selectors in the clipboard module. The intention was to remove most of the
@ts-expect-errors in the file and to make it more clear what kind of nodes each matcher could possibly be used for. It also prevents mixing a matcher with the wrong kind of node.This PR is not expected to change any runtime behavior.