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

[TS migration] Migrate 'SelectionScraper' lib to TypeScript #27796

Merged
4 changes: 0 additions & 4 deletions src/libs/SelectionScraper/index.native.js

This file was deleted.

8 changes: 8 additions & 0 deletions src/libs/SelectionScraper/index.native.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import GetCurrentSelection from './types';

// This is a no-op function for native devices because they wouldn't be able to support Selection API like a website.
const getCurrentSelection: GetCurrentSelection = () => '';

export default {
getCurrentSelection,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Add types.ts file with common types for getCurrentSelection

Original file line number Diff line number Diff line change
@@ -1,25 +1,27 @@
import render from 'dom-serializer';
import {parseDocument} from 'htmlparser2';
import _ from 'underscore';
import Str from 'expensify-common/lib/str';
import {DataNode, Element, Node} from 'domhandler';
import CONST from '../../CONST';
import GetCurrentSelection from './types';

const elementsWillBeSkipped = ['html', 'body'];
const tagAttribute = 'data-testid';

/**
* Reads html of selection. If browser doesn't support Selection API, returns empty string.
* @returns {String} HTML of selection as String
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @returns {String} HTML of selection as String
* @returns HTML of selection as String

* @returns HTML of selection as String
*/
const getHTMLOfSelection = () => {
const getHTMLOfSelection = (): string => {
// If browser doesn't support Selection API, return an empty string.
if (!window.getSelection) {
const selection = window.getSelection();
Copy link
Contributor

Choose a reason for hiding this comment

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

won't this throw if we don't have window.getSelection(); ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Skalakid is OOO this week, he will adjust it once he's back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the Window types, getSelection should be always present. So I think it won't throw


if (!selection || !window.getSelection) {
return '';
}
const selection = window.getSelection();

if (selection.rangeCount <= 0) {
return window.getSelection().toString();
return window.getSelection()?.toString() ?? '';
}

const div = document.createElement('div');
Expand All @@ -44,7 +46,7 @@ const getHTMLOfSelection = () => {

// If clonedSelection has no text content this data has no meaning to us.
if (clonedSelection.textContent) {
let parent;
let parent: globalThis.Element | null = null;
let child = clonedSelection;

// If selection starts and ends within same text node we use its parentNode. This is because we can't
Expand All @@ -64,17 +66,17 @@ const getHTMLOfSelection = () => {
// and finally commonAncestorContainer.parentNode.closest('data-testid') is targeted dom.
if (range.commonAncestorContainer instanceof HTMLElement) {
parent = range.commonAncestorContainer.closest(`[${tagAttribute}]`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Type parent, now it's of type any

} else {
parent = range.commonAncestorContainer.parentNode.closest(`[${tagAttribute}]`);
} else if (range.commonAncestorContainer.parentNode) {
parent = (range.commonAncestorContainer.parentNode as HTMLElement).closest(`[${tagAttribute}]`);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} else if (range.commonAncestorContainer.parentNode) {
parent = (range.commonAncestorContainer.parentNode as HTMLElement).closest(`[${tagAttribute}]`);
}
} else {
parent = (range.commonAncestorContainer.parentNode as HTMLElement | null)?.closest(`[${tagAttribute}]`) ?? null;
}

Would be safer if we do like this, in this way we are not changing conditional flow.


// Keep traversing up to clone all parents with 'data-testid' attribute.
while (parent) {
const cloned = parent.cloneNode();
cloned.appendChild(child);
child = cloned;
child = cloned as DocumentFragment;

parent = parent.parentNode.closest(`[${tagAttribute}]`);
parent = (parent.parentNode as HTMLElement).closest(`[${tagAttribute}]`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
parent = (parent.parentNode as HTMLElement).closest(`[${tagAttribute}]`);
parent = (parent.parentNode as HTMLElement | null)?.closest(`[${tagAttribute}]`) ?? null;

parentNode can be null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks!

}

div.appendChild(child);
Expand All @@ -96,58 +98,61 @@ const getHTMLOfSelection = () => {

/**
* Clears all attributes from dom elements
* @param {Object} dom htmlparser2 dom representation
* @param {Boolean} isChildOfEditorElement
* @returns {Object} htmlparser2 dom representation
*/
const replaceNodes = (dom, isChildOfEditorElement) => {
let domName = dom.name;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for this change?

Copy link
Contributor

@blazejkustra blazejkustra Oct 6, 2023

Choose a reason for hiding this comment

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

name is only available on Element

let domChildren;
const domAttribs = {};
let data;
const replaceNodes = (dom: Node, isChildOfEditorElement: boolean): Node => {
Copy link
Member

Choose a reason for hiding this comment

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

The return type does not match the return value. Let's use one type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Node is a parent class of DataNode and Element, this function has to be more generic that's why Node is the return type

const domElement = dom as Element;
let domName = domElement.name;
let domChildren: Node[] = [];
const domAttribs: Element['attribs'] = {};
let data = '';

// Encoding HTML chars '< >' in the text, because any HTML will be removed in stripHTML method.
if (dom.type === 'text') {
data = Str.htmlEncode(dom.data);
if (dom.type.toString() === 'text') {
data = Str.htmlEncode((dom as DataNode).data);
}

// We are skipping elements which has html and body in data-testid, since ExpensiMark can't parse it. Also this data
// has no meaning for us.
if (dom.attribs && dom.attribs[tagAttribute]) {
if (!elementsWillBeSkipped.includes(dom.attribs[tagAttribute])) {
domName = dom.attribs[tagAttribute];
if (domElement.attribs?.[tagAttribute]) {
if (!elementsWillBeSkipped.includes(domElement.attribs[tagAttribute])) {
domName = domElement.attribs[tagAttribute];
}
} else if (dom.name === 'div' && dom.children.length === 1 && isChildOfEditorElement) {
} else if (domElement.name === 'div' && domElement.children.length === 1 && isChildOfEditorElement) {
// We are excluding divs that are children of our editor element and have only one child to prevent
// additional newlines from being added in the HTML to Markdown conversion process.
return replaceNodes(dom.children[0], isChildOfEditorElement);
return replaceNodes(domElement.children[0], isChildOfEditorElement);
}

// We need to preserve href attribute in order to copy links.
if (dom.attribs && dom.attribs.href) {
domAttribs.href = dom.attribs.href;
if (domElement.attribs?.href) {
domAttribs.href = domElement.attribs.href;
}

if (domElement.children) {
domChildren = domElement.children.map((c) => replaceNodes(c, isChildOfEditorElement || !!domElement.attribs?.[tagAttribute]));
}

if (dom.children) {
domChildren = _.map(dom.children, (c) => replaceNodes(c, isChildOfEditorElement || !_.isEmpty(dom.attribs && dom.attribs[tagAttribute])));
if (data) {
return {
...dom,
data,
} as DataNode;
}

return {
...dom,
data,
name: domName,
attribs: domAttribs,
children: domChildren,
};
} as Element;
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't fully understand why this change was made.

Copy link
Contributor Author

@Skalakid Skalakid Oct 2, 2023

Choose a reason for hiding this comment

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

I made this change because replaceNodes function should return Node type elements. Node type is a general type and there are many more specific ones that extend it. The problem was that children can be different types of Nodes. According to the domhandler types there isn't any type that have both data and attribs/children attributes. So if there is Node have data I'm returning DataNode, if no I'm returning Element type Node

};

/**
* Resolves the current selection to values and produces clean HTML.
* @returns {String} resolved selection in the HTML format
*/
const getCurrentSelection = () => {
const getCurrentSelection: GetCurrentSelection = () => {
const domRepresentation = parseDocument(getHTMLOfSelection());
domRepresentation.children = _.map(domRepresentation.children, replaceNodes);
domRepresentation.children = domRepresentation.children.map((item) => replaceNodes(item, false));

// Newline characters need to be removed here because the HTML could contain both newlines and <br> tags, and when
// <br> tags are converted later to markdown, it creates duplicate newline characters. This means that when the content
Expand Down
3 changes: 3 additions & 0 deletions src/libs/SelectionScraper/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
type GetCurrentSelection = () => string;

export default GetCurrentSelection;
Copy link
Member

Choose a reason for hiding this comment

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

Normally, the name of the file matches with the default export but here it isn't. Let's convert it to named export.

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually in other libstypes.ts files we used default export, so for consistency I would leave it as is @parasharrajat

Loading