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.

Sign up for GitHub

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

Make matchers more strongly typed #4083

Closed
wants to merge 6 commits into from
Closed

Make matchers more strongly typed #4083

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a283665
Make matchers more strongly typed
justinbhopper Mar 25, 2024
6623b72
Fix lint error
justinbhopper Mar 26, 2024
16aa1da
Merge remote-tracking branch 'upstream/main'
justinbhopper Mar 26, 2024
2a384ed
Merge branch 'main' into matcher-types
justinbhopper Mar 26, 2024
bb56346
Fix broken isPre check
justinbhopper Mar 26, 2024
6991d11
Remove NodeSelector and NodeMatcher types
justinbhopper Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 66 additions & 46 deletions packages/quill/src/modules/clipboard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,23 @@ import normalizeExternalHTML from './normalizeExternalHTML/index.js';

const debug = logger('quill:clipboard');

type Selector = string | Node['TEXT_NODE'] | Node['ELEMENT_NODE'];
type Matcher = (node: Node, delta: Delta, scroll: ScrollBlot) => Delta;
type TextSelector = Node['TEXT_NODE'];
type ElementSelector = string | Node['ELEMENT_NODE'];
type Selector = TextSelector | ElementSelector;

const CLIPBOARD_CONFIG: [Selector, Matcher][] = [
type TextMatcher = (node: Text, delta: Delta, scroll: ScrollBlot) => Delta;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay no problem, then probably this PR would do more harm than good so I'll close it out!

type ElementMatcher = (
node: Element,
delta: Delta,
scroll: ScrollBlot,
) => Delta;
type Matcher = TextMatcher | ElementMatcher;

type MatcherConfig =
| [TextSelector, TextMatcher]
| [ElementSelector, ElementMatcher];

const CLIPBOARD_CONFIG: MatcherConfig[] = [
[Node.TEXT_NODE, matchText],
[Node.TEXT_NODE, matchNewline],
['br', matchBreak],
Expand Down Expand Up @@ -68,15 +81,15 @@ const STYLE_ATTRIBUTORS = [
}, {});

interface ClipboardOptions {
matchers: [Selector, Matcher][];
matchers: MatcherConfig[];
}

class Clipboard extends Module<ClipboardOptions> {
static DEFAULTS: ClipboardOptions = {
matchers: [],
};

matchers: [Selector, Matcher][];
matchers: MatcherConfig[];

constructor(quill: Quill, options: Partial<ClipboardOptions>) {
super(quill, options);
Expand All @@ -86,15 +99,15 @@ class Clipboard extends Module<ClipboardOptions> {
this.quill.root.addEventListener('cut', (e) => this.onCaptureCopy(e, true));
this.quill.root.addEventListener('paste', this.onCapturePaste.bind(this));
this.matchers = [];
CLIPBOARD_CONFIG.concat(this.options.matchers ?? []).forEach(
([selector, matcher]) => {
this.addMatcher(selector, matcher);
},
);
CLIPBOARD_CONFIG.concat(this.options.matchers ?? []).forEach((config) => {
this.matchers.push(config);
});
}

addMatcher(selector: Selector, matcher: Matcher) {
this.matchers.push([selector, matcher]);
addMatcher(selector: TextSelector, matcher: TextMatcher): void;
addMatcher(selector: ElementSelector, matcher: ElementMatcher): void;
addMatcher(selector: Selector, matcher: Matcher): void {
this.matchers.push([selector, matcher] as MatcherConfig);
}

convert(
Expand Down Expand Up @@ -128,17 +141,17 @@ class Clipboard extends Module<ClipboardOptions> {
const doc = new DOMParser().parseFromString(html, 'text/html');
this.normalizeHTML(doc);
const container = doc.body;
const nodeMatches = new WeakMap();
const queryMatches = new WeakMap<Element, ElementMatcher[]>();
const [elementMatchers, textMatchers] = this.prepareMatching(
container,
nodeMatches,
queryMatches,
);
return traverse(
this.quill.scroll,
container,
elementMatchers,
textMatchers,
nodeMatches,
queryMatches,
);
}

Expand Down Expand Up @@ -249,9 +262,12 @@ class Clipboard extends Module<ClipboardOptions> {
this.quill.scrollSelectionIntoView();
}

prepareMatching(container: Element, nodeMatches: WeakMap<Node, Matcher[]>) {
const elementMatchers: Matcher[] = [];
const textMatchers: Matcher[] = [];
prepareMatching(
container: Element,
queryMatches: WeakMap<Element, ElementMatcher[]>,
): [elementMatchers: ElementMatcher[], textMatchers: TextMatcher[]] {
const elementMatchers: ElementMatcher[] = [];
const textMatchers: TextMatcher[] = [];
this.matchers.forEach((pair) => {
const [selector, matcher] = pair;
switch (selector) {
Expand All @@ -263,11 +279,11 @@ class Clipboard extends Module<ClipboardOptions> {
break;
default:
Array.from(container.querySelectorAll(selector)).forEach((node) => {
if (nodeMatches.has(node)) {
const matches = nodeMatches.get(node);
if (queryMatches.has(node)) {
const matches = queryMatches.get(node);
matches?.push(matcher);
} else {
nodeMatches.set(node, [matcher]);
queryMatches.set(node, [matcher]);
}
});
break;
Expand Down Expand Up @@ -355,7 +371,7 @@ function isLine(node: Node, scroll: ScrollBlot) {
].includes(node.tagName.toLowerCase());
}

function isBetweenInlineElements(node: HTMLElement, scroll: ScrollBlot) {
function isBetweenInlineElements(node: Text | Element, scroll: ScrollBlot) {
return (
node.previousElementSibling &&
node.nextElementSibling &&
Expand All @@ -364,47 +380,54 @@ function isBetweenInlineElements(node: HTMLElement, scroll: ScrollBlot) {
);
}

const preNodes = new WeakMap();
function isPre(node: Node | null) {
const preNodes = new WeakMap<Node, boolean>();
function isPre(node: Node | null): node is HTMLPreElement {
if (node == null) return false;
if (!preNodes.has(node)) {
// @ts-expect-error
if (node.tagName === 'PRE') {
if (isElement(node) && node.tagName === 'PRE') {
preNodes.set(node, true);
} else {
preNodes.set(node, isPre(node.parentNode));
}
}
return preNodes.get(node);
return !!preNodes.get(node);
}

function isText(node: Node): node is Text {
return node.nodeType === node.TEXT_NODE;
}

function isElement(node: Node): node is Element {
return node.nodeType === node.ELEMENT_NODE;
}

function traverse(
scroll: ScrollBlot,
node: ChildNode,
elementMatchers: Matcher[],
textMatchers: Matcher[],
nodeMatches: WeakMap<Node, Matcher[]>,
elementMatchers: ElementMatcher[],
textMatchers: TextMatcher[],
queryMatches: WeakMap<Element, ElementMatcher[]>,
): Delta {
// Post-order
if (node.nodeType === node.TEXT_NODE) {
if (isText(node)) {
return textMatchers.reduce((delta: Delta, matcher) => {
return matcher(node, delta, scroll);
}, new Delta());
}
if (node.nodeType === node.ELEMENT_NODE) {
if (isElement(node)) {
return Array.from(node.childNodes || []).reduce((delta, childNode) => {
let childrenDelta = traverse(
scroll,
childNode,
elementMatchers,
textMatchers,
nodeMatches,
queryMatches,
);
if (childNode.nodeType === node.ELEMENT_NODE) {
if (isElement(childNode)) {
childrenDelta = elementMatchers.reduce((reducedDelta, matcher) => {
return matcher(childNode as HTMLElement, reducedDelta, scroll);
return matcher(childNode, reducedDelta, scroll);
}, childrenDelta);
childrenDelta = (nodeMatches.get(childNode) || []).reduce(
childrenDelta = (queryMatches.get(childNode) || []).reduce(
(reducedDelta, matcher) => {
return matcher(childNode, reducedDelta, scroll);
},
Expand Down Expand Up @@ -454,7 +477,7 @@ function matchAttributor(node: HTMLElement, delta: Delta, scroll: ScrollBlot) {
);
}

function matchBlot(node: Node, delta: Delta, scroll: ScrollBlot) {
function matchBlot(node: Element, delta: Delta, scroll: ScrollBlot) {
const match = scroll.query(node);
if (match == null) return delta;
// @ts-expect-error
Expand Down Expand Up @@ -522,8 +545,7 @@ function matchIndent(node: Node, delta: Delta, scroll: ScrollBlot) {
let indent = -1;
let parent = node.parentNode;
while (parent != null) {
// @ts-expect-error
if (['OL', 'UL'].includes(parent.tagName)) {
if (isElement(parent) && ['OL', 'UL'].includes(parent.tagName)) {
indent += 1;
}
parent = parent.parentNode;
Expand All @@ -538,19 +560,18 @@ function matchIndent(node: Node, delta: Delta, scroll: ScrollBlot) {
}, new Delta());
}

function matchList(node: Node, delta: Delta, scroll: ScrollBlot) {
const element = node as Element;
let list = element.tagName === 'OL' ? 'ordered' : 'bullet';
function matchList(node: Element, delta: Delta, scroll: ScrollBlot) {
let list = node.tagName === 'OL' ? 'ordered' : 'bullet';

const checkedAttr = element.getAttribute('data-checked');
const checkedAttr = node.getAttribute('data-checked');
if (checkedAttr) {
list = checkedAttr === 'true' ? 'checked' : 'unchecked';
}

return applyFormat(delta, 'list', list, scroll);
}

function matchNewline(node: Node, delta: Delta, scroll: ScrollBlot) {
function matchNewline(node: Text | Element, delta: Delta, scroll: ScrollBlot) {
if (!deltaEndsWith(delta, '\n')) {
if (
isLine(node, scroll) &&
Expand Down Expand Up @@ -624,8 +645,7 @@ function matchTable(
return delta;
}

function matchText(node: HTMLElement, delta: Delta, scroll: ScrollBlot) {
// @ts-expect-error
function matchText(node: Text, delta: Delta, scroll: ScrollBlot) {
let text = node.data;
// Word represents empty line with <o:p>&nbsp;</o:p>
if (node.parentElement?.tagName === 'O:P') {
Expand Down
1 change: 0 additions & 1 deletion packages/quill/src/modules/syntax.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,6 @@ class Syntax extends Module<SyntaxOptions> {
],
[
(node, delta) => {
// @ts-expect-error
return node.data.split('\n').reduce((memo, nodeText, i) => {
if (i !== 0) memo.insert('\n', { [CodeBlock.blotName]: language });
return memo.insert(nodeText);
Expand Down
Loading