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,29 @@
import render from 'dom-serializer';
import Str from 'expensify-common/lib/str';
import {parseDocument} from 'htmlparser2';
import _ from 'underscore';
import {DataNode, Element, Node} from 'domhandler';
import CONST from '@src/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) {
return '';
}
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) {
return '';
}

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

const div = document.createElement('div');
Expand All @@ -44,7 +48,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 @@ -65,16 +69,16 @@ const getHTMLOfSelection = () => {
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}]`);
parent = (range.commonAncestorContainer.parentNode as HTMLElement | null)?.closest(`[${tagAttribute}]`) ?? null;
}

// 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 | null)?.closest(`[${tagAttribute}]`) ?? null;
}

div.appendChild(child);
Expand All @@ -96,40 +100,41 @@ const getHTMLOfSelection = () => {

/**
* Clears all attributes from dom elements
* @param {Object} dom htmlparser2 dom representation
* @param {Boolean} isChildOfEditorElement
* @returns {Object} htmlparser2 dom representation
* @param dom - dom 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

let domName;
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') {
if (dom.type.toString() === 'text' && dom instanceof DataNode) {
Copy link
Member

Choose a reason for hiding this comment

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

Previously we were strictly matching type property. Now we are doing a type conversion before matching it. I don't know if it changes anything logically but why do we have to change this condition? I don't think we should change it just to satisfy type checks.

We defined dom as Node on the function definition and now we are checking the type of DataNode. Either we should define the correct type or not.

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.

Previously we were strictly matching type property. Now we are doing a type conversion before matching it. I don't know if it changes anything logically but why do we have to change this condition?

@Skalakid Use this instead:

import { ElementType } from "domelementtype";
...

if (dom.type === ElementType.Text && dom instanceof DataNode) {

We defined dom as Node on the function definition and now we are checking the type of DataNode. Either we should define the correct type or not.

DataNode is a child of Node, so it's perfectly fine to narrow down the type like we do in the if statement dom instanceof DataNode @parasharrajat, wdyt?

dom instanceof DataNode is needed to narrow down the type & use its additional properties that aren't available on Node

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Thanks.

data = Str.htmlEncode(dom.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];
} else if (dom instanceof Element) {
domName = dom.name;
// 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?.[tagAttribute]) {
if (!elementsWillBeSkipped.includes(dom.attribs[tagAttribute])) {
domName = dom.attribs[tagAttribute];
}
} else if (dom.name === 'div' && dom.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);
}
} else if (dom.name === 'div' && dom.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);
}

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

if (dom.children) {
domChildren = _.map(dom.children, (c) => replaceNodes(c, isChildOfEditorElement || !_.isEmpty(dom.attribs && dom.attribs[tagAttribute])));
if (dom.children) {
domChildren = dom.children.map((c) => replaceNodes(c, isChildOfEditorElement || !!dom.attribs?.[tagAttribute]));
}
} else {
throw new Error(`Unknown dom type: ${dom.type}`);
}

return {
Expand All @@ -138,16 +143,15 @@ const replaceNodes = (dom, isChildOfEditorElement) => {
name: domName,
attribs: domAttribs,
children: domChildren,
};
} as Element & DataNode;
};

/**
* 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