From dc247c3f5386251be7be27221249935040ba9d77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Jasikowski?= Date: Thu, 26 Sep 2024 14:30:34 +0200 Subject: [PATCH 1/5] Added img attribute caching to ExpensiMark.ts, deprecated cacheVideoAttributes & videoAttributeCache --- __tests__/ExpensiMark-HTML-test.js | 39 +++++++++++- __tests__/ExpensiMark-Markdown-test.js | 44 +++++++++++++ lib/ExpensiMark.ts | 87 +++++++++++++++++++++----- 3 files changed, 155 insertions(+), 15 deletions(-) diff --git a/__tests__/ExpensiMark-HTML-test.js b/__tests__/ExpensiMark-HTML-test.js index 1417cc5a..eda96c1b 100644 --- a/__tests__/ExpensiMark-HTML-test.js +++ b/__tests__/ExpensiMark-HTML-test.js @@ -2097,7 +2097,7 @@ describe('Video markdown conversion to html tag', () => { expect(parser.replace(testString, {shouldKeepRawInput: true})).toBe(resultString); }) - test('Single video with extra cached attribues', () => { + test('Single video with extra cached attributes with videoAttributeCache', () => { const testString = '![test](https://example.com/video.mp4)'; const resultString = ''; expect(parser.replace(testString, { @@ -2109,6 +2109,18 @@ describe('Video markdown conversion to html tag', () => { })).toBe(resultString); }) + test('Single video with extra cached attributes with mediaAttributeCache', () => { + const testString = '![test](https://example.com/video.mp4)'; + const resultString = ''; + expect(parser.replace(testString, { + extras: { + mediaAttributeCache: { + 'https://example.com/video.mp4': 'data-expensify-height="100" data-expensify-width="100"' + } + } + })).toBe(resultString); + }) + test('Text containing image and video', () => { const testString = 'An image of a banana: ![banana](https://example.com/banana.png) and a video of a banana: ![banana](https://example.com/banana.mp4)'; const resultString = 'An image of a banana: banana and a video of a banana: '; @@ -2220,6 +2232,31 @@ describe('Image markdown conversion to html tag', () => { '```code```'; expect(parser.replace(testString, {shouldKeepRawInput: true})).toBe(resultString); }); + + test('Single image with extra cached attributes using mediaAttributeCache', () => { + const testString = '![test](https://example.com/image.jpg)'; + const resultString = 'test'; + expect(parser.replace(testString, { + extras: { + mediaAttributeCache: { + 'https://example.com/image.jpg': 'data-expensify-height="100" data-expensify-width="100"' + } + } + })).toBe(resultString); + }) + + test('Single image with extra cached attributes using videAttributeCache', () => { + const testString = '![test](https://example.com/image.jpg)'; + const resultString = 'test'; + expect(parser.replace(testString, { + extras: { + videoAttributeCache: { + 'https://example.com/image.jpg': 'data-expensify-height="100" data-expensify-width="100"' + } + } + })).toBe(resultString); + }) + }); describe('room mentions', () => { diff --git a/__tests__/ExpensiMark-Markdown-test.js b/__tests__/ExpensiMark-Markdown-test.js index 1ebfd21a..6daaab21 100644 --- a/__tests__/ExpensiMark-Markdown-test.js +++ b/__tests__/ExpensiMark-Markdown-test.js @@ -867,6 +867,50 @@ describe('Image tag conversion to markdown', () => { const resultString = '![```code```](https://example.com/image.png)'; expect(parser.htmlToMarkdown(testString)).toBe(resultString); }); + + test('Cache extra attributes for img with alt with mediaAttributeCachingFn', () => { + const mediaAttributeCachingFn = jest.fn(); + const testString = 'altText'; + const resultString = '![altText](https://example.com/image.png)'; + const extras = { + mediaAttributeCachingFn, + }; + expect(parser.htmlToMarkdown(testString, extras)).toBe(resultString); + expect(mediaAttributeCachingFn).toHaveBeenCalledWith("https://example.com/image.png", 'data-expensify-width="100" data-expensify-height="500" data-name="newName" data-expensify-source="expensify-source"') + }); + + test('Cache extra attributes for img with alt with cacheVideoAttributes', () => { + const cacheVideoAttributes = jest.fn(); + const testString = 'altText'; + const resultString = '![altText](https://example.com/image.png)'; + const extras = { + cacheVideoAttributes, + }; + expect(parser.htmlToMarkdown(testString, extras)).toBe(resultString); + expect(cacheVideoAttributes).toHaveBeenCalledWith("https://example.com/image.png", 'data-expensify-width="100" data-expensify-height="500" data-name="newName" data-expensify-source="expensify-source"') + }); + + test('Cache extra attributes for img without alt with mediaAttributeCachingFn', () => { + const mediaAttributeCachingFn = jest.fn(); + const testString = ''; + const resultString = '!(https://example.com/image.png)'; + const extras = { + mediaAttributeCachingFn, + }; + expect(parser.htmlToMarkdown(testString, extras)).toBe(resultString); + expect(mediaAttributeCachingFn).toHaveBeenCalledWith("https://example.com/image.png", 'data-expensify-width="100" data-expensify-height="500" data-name="newName" data-expensify-source="expensify-source"') + }); + + test('Cache extra attributes for img without alt with cacheVideoAttributes', () => { + const cacheVideoAttributes = jest.fn(); + const testString = ''; + const resultString = '!(https://example.com/image.png)'; + const extras = { + cacheVideoAttributes, + }; + expect(parser.htmlToMarkdown(testString, extras)).toBe(resultString); + expect(cacheVideoAttributes).toHaveBeenCalledWith("https://example.com/image.png", 'data-expensify-width="100" data-expensify-height="500" data-name="newName" data-expensify-source="expensify-source"') + }); }); describe('Video tag conversion to markdown', () => { diff --git a/lib/ExpensiMark.ts b/lib/ExpensiMark.ts index 0a17a763..680119cc 100644 --- a/lib/ExpensiMark.ts +++ b/lib/ExpensiMark.ts @@ -1,3 +1,5 @@ +import str from "./str"; + 'worklet'; import Str from './str'; @@ -9,8 +11,16 @@ import * as Utils from './utils'; type Extras = { reportIDToName?: Record; accountIDToName?: Record; + /** + * @deprecated Replaced with mediaAttributeCachingFn + */ cacheVideoAttributes?: (vidSource: string, attrs: string) => void; + /** + * @deprecated Replaced with mediaAttributeCache + */ videoAttributeCache?: Record; + mediaAttributeCachingFn?: (mediaSource: string, attrs: string) => void; + mediaAttributeCache?: Record; }; const EXTRAS_DEFAULT = {}; @@ -62,6 +72,18 @@ const MARKDOWN_VIDEO_REGEX = new RegExp( const SLACK_SPAN_NEW_LINE_TAG = ''; export default class ExpensiMark { + + getAttributeCache = (extras?: Extras) => { + if (!extras) { + return { attrCachingFn: undefined, attrCache: undefined }; + } + + return { + attrCachingFn: extras.mediaAttributeCachingFn ?? extras.cacheVideoAttributes, + attrCache: extras.mediaAttributeCache ?? extras.videoAttributeCache + }; + }; + static Log = new Logger({ serverLoggingCallback: () => undefined, // eslint-disable-next-line no-console @@ -171,11 +193,13 @@ export default class ExpensiMark { * @return Returns the HTML video tag */ replacement: (extras, _match, videoName, videoSource) => { - const extraAttrs = extras && extras.videoAttributeCache && extras.videoAttributeCache[videoSource]; + const attrCache = this.getAttributeCache(extras).attrCache; + const extraAttrs = attrCache && attrCache[videoSource]; return ``; }, rawInputReplacement: (extras, _match, videoName, videoSource) => { - const extraAttrs = extras && extras.videoAttributeCache && extras.videoAttributeCache[videoSource]; + const attrCache = this.getAttributeCache(extras).attrCache; + const extraAttrs = attrCache && attrCache[videoSource]; return ``; }, }, @@ -245,13 +269,21 @@ export default class ExpensiMark { * tag. * Additional sanitization is done to the alt attribute to prevent parsing it further to html by later * rules. + * Additional tags from cache are applied to the result HTML. */ { name: 'image', regex: MARKDOWN_IMAGE_REGEX, - replacement: (_extras, _match, g1, g2) => `${this.escapeAttributeContent(g1)}`, - rawInputReplacement: (_extras, _match, g1, g2) => - `${this.escapeAttributeContent(g1)}`, + replacement: (extras, _match, imgAlt, imgSource) => { + const attrCache = this.getAttributeCache(extras).attrCache; + const extraAttrs = attrCache && attrCache[imgSource]; + return `${this.escapeAttributeContent(imgAlt)}`; + }, + rawInputReplacement: (extras, _match, imgAlt, imgSource) => { + const attrCache = this.getAttributeCache(extras).attrCache; + const extraAttrs = attrCache && attrCache[imgSource]; + return `${this.escapeAttributeContent(imgAlt)}`; + }, }, /** @@ -658,14 +690,39 @@ export default class ExpensiMark { { name: 'image', - regex: /<]*src\s*=\s*(['"])(.*?)\1(?:[^><]*alt\s*=\s*(['"])(.*?)\3)?[^><]*>*(?![^<][\s\S]*?(<\/pre>|<\/code>))/gi, - replacement: (_extras, _match, _g1, g2, _g3, g4) => { - if (g4) { - return `![${g4}](${g2})`; + regex: /<]*src\s*=\s*(['"])(.*?)\1(.*?)>(?![^<][\s\S]*?(<\/pre>|<\/code>))/gi, + /** + * @param extras - The extras object + * @param match - The full match + * @param _g1 - The first capture group (the quote) + * @param imgSource - The second capture group - src attribute value + * @param imgAttrs - The third capture group - any attributes after src + * @returns The markdown image tag + */ + replacement: (extras, _match, _g1, imgSource, imgAttrs) => { + // Extract alt attribute from imgAttrs + let altText = ''; + const altRegex = /alt\s*=\s*(['"])(.*?)\1/i; + const altMatch = imgAttrs.match(altRegex); + const attrCachingFn = this.getAttributeCache(extras).attrCachingFn; + let attributes = imgAttrs; + if (altMatch) { + altText = altMatch[2]; + // Remove the alt attribute from imgAttrs + attributes = attributes.replace(altRegex, ''); } - - return `!(${g2})`; - }, + // Remove trailing slash and extra whitespace + attributes = attributes.replace(/\s*\/\s*$/, '').trim(); + // Cache attributes without alt and trailing slash + if (imgAttrs && attrCachingFn && typeof attrCachingFn === 'function') { + attrCachingFn(imgSource, attributes); + } + // Return the markdown image tag + if (altText) { + return `![${altText}](${imgSource})`; + } + return `!(${imgSource})`; + } }, { @@ -681,8 +738,10 @@ export default class ExpensiMark { * @returns The markdown video tag */ replacement: (extras, _match, _g1, videoSource, videoAttrs, videoName) => { - if (videoAttrs && extras && extras.cacheVideoAttributes && typeof extras.cacheVideoAttributes === 'function') { - extras.cacheVideoAttributes(videoSource, videoAttrs); + const attrCachingFn = this.getAttributeCache(extras).attrCachingFn; + + if (videoAttrs && attrCachingFn && typeof attrCachingFn === 'function') { + attrCachingFn(videoSource, videoAttrs); } if (videoName) { return `![${videoName}](${videoSource})`; From 422f1b6f5effd945f9d94ab1237a466b66a3950b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Jasikowski?= Date: Thu, 26 Sep 2024 14:38:03 +0200 Subject: [PATCH 2/5] Added additional comments to mediaAttributeCachingFn and mediaAttributeCache --- lib/ExpensiMark.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/ExpensiMark.ts b/lib/ExpensiMark.ts index 680119cc..6ab7a7e5 100644 --- a/lib/ExpensiMark.ts +++ b/lib/ExpensiMark.ts @@ -19,7 +19,15 @@ type Extras = { * @deprecated Replaced with mediaAttributeCache */ videoAttributeCache?: Record; + /** + * Function used to cache HTML tag attributes during conversion to and from Markdown + * @param mediaSource URI to media source + * @param attrs Additional attributes to be cached + */ mediaAttributeCachingFn?: (mediaSource: string, attrs: string) => void; + /** + * Key/value cache for HTML tag attributes, where the key is media source URI, value is cached attributes + */ mediaAttributeCache?: Record; }; const EXTRAS_DEFAULT = {}; From d30ed4e72eae69ff58caf08fbf249e6ebe9037b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Jasikowski?= Date: Thu, 26 Sep 2024 15:05:40 +0200 Subject: [PATCH 3/5] Added more media caching tests to ExpensiMark --- __tests__/ExpensiMark-Markdown-test.js | 13 ++++++- __tests__/ExpensiMark-test.js | 53 ++++++++++++++++++++++++++ lib/ExpensiMark.ts | 4 +- 3 files changed, 68 insertions(+), 2 deletions(-) diff --git a/__tests__/ExpensiMark-Markdown-test.js b/__tests__/ExpensiMark-Markdown-test.js index 6daaab21..0a70d3de 100644 --- a/__tests__/ExpensiMark-Markdown-test.js +++ b/__tests__/ExpensiMark-Markdown-test.js @@ -926,7 +926,7 @@ describe('Video tag conversion to markdown', () => { expect(parser.htmlToMarkdown(testString)).toBe(resultString); }) - test('While convert video, cache some extra attributes from the video tag', () => { + test('Video with extra attributes to be cached with cacheVideoAttributes', () => { const cacheVideoAttributes = jest.fn(); const testString = ''; const resultString = '![video](https://example.com/video.mp4)'; @@ -936,6 +936,17 @@ describe('Video tag conversion to markdown', () => { expect(parser.htmlToMarkdown(testString, extras)).toBe(resultString); expect(cacheVideoAttributes).toHaveBeenCalledWith("https://example.com/video.mp4", ' data-expensify-width="100" data-expensify-height="500" data-expensify-thumbnail-url="https://image.com/img.jpg"') }) + + test('Video with extra attributes to be cached with mediaAttributeCachingFn', () => { + const mediaAttributeCachingFn = jest.fn(); + const testString = ''; + const resultString = '![video](https://example.com/video.mp4)'; + const extras = { + mediaAttributeCachingFn, + }; + expect(parser.htmlToMarkdown(testString, extras)).toBe(resultString); + expect(mediaAttributeCachingFn).toHaveBeenCalledWith("https://example.com/video.mp4", ' data-expensify-width="100" data-expensify-height="500" data-expensify-thumbnail-url="https://image.com/img.jpg"') + }) }) describe('Tag names starting with common charaters', () => { diff --git a/__tests__/ExpensiMark-test.js b/__tests__/ExpensiMark-test.js index 31983401..41c0a2c9 100644 --- a/__tests__/ExpensiMark-test.js +++ b/__tests__/ExpensiMark-test.js @@ -1,6 +1,7 @@ /* eslint-disable max-len */ import ExpensiMark from '../lib/ExpensiMark'; import * as Utils from '../lib/utils'; +import {any, string} from "prop-types"; const parser = new ExpensiMark(); @@ -48,3 +49,55 @@ test('Test extract link from Markdown link syntax', () => { const links = ['https://new.expensify.com/']; expect(parser.extractLinksInMarkdownComment(comment)).toStrictEqual(links); }); + +describe('Test ExpensiMark getAttributeCache', () => { + const expensiMark = new ExpensiMark(); + + describe('For attrCachingFn', () => { + test('If mediaAttributeCachingFn is provided, returns it', () => { + const extras = { + mediaAttributeCachingFn: jest.fn(), + } + expect(expensiMark.getAttributeCache(extras).attrCachingFn).toBe(extras.mediaAttributeCachingFn); + }) + + test('If mediaAttributeCachingFn is not provided, returns cacheVideoAttributes', () => { + const extras = { + cacheVideoAttributes: jest.fn(), + } + expect(expensiMark.getAttributeCache(extras).attrCachingFn).toBe(extras.cacheVideoAttributes); + }) + + test('If mediaAttributeCachingFn and cacheVideoAttributes are not provided, returns undefined', () => { + const extras = {} + expect(expensiMark.getAttributeCache(extras).attrCachingFn).toBe(undefined); + }) + }); + + describe('For attrCache', () => { + test('If mediaAttributeCache is provided, returns it', () => { + const extras = { + mediaAttributeCache: jest.fn(), + } + expect(expensiMark.getAttributeCache(extras).attrCache).toBe(extras.mediaAttributeCache); + }) + + test('If mediaAttributeCache is not provided, returns videoAttributeCache', () => { + const extras = { + videoAttributeCache: jest.fn(), + } + expect(expensiMark.getAttributeCache(extras).attrCache).toBe(extras.videoAttributeCache); + }) + + test('If mediaAttributeCache and videoAttributeCache are not provided, returns undefined', () => { + const extras = {} + expect(expensiMark.getAttributeCache(extras).attrCache).toBe(undefined); + }) + }); + + test('If no extras are undefined, returns undefined for both attrCachingFn and attrCache', () => { + const {attrCachingFn, attrCache} = expensiMark.getAttributeCache(undefined); + expect(attrCachingFn).toBe(undefined); + expect(attrCache).toBe(undefined); + }) +}); diff --git a/lib/ExpensiMark.ts b/lib/ExpensiMark.ts index 6ab7a7e5..74299cad 100644 --- a/lib/ExpensiMark.ts +++ b/lib/ExpensiMark.ts @@ -30,6 +30,8 @@ type Extras = { */ mediaAttributeCache?: Record; }; +export type {Extras}; + const EXTRAS_DEFAULT = {}; type ReplacementFn = (extras: Extras, ...matches: string[]) => string; @@ -83,7 +85,7 @@ export default class ExpensiMark { getAttributeCache = (extras?: Extras) => { if (!extras) { - return { attrCachingFn: undefined, attrCache: undefined }; + return {attrCachingFn: undefined, attrCache: undefined}; } return { From bf3d9a042dc3e084859ddf6ffff7c1aa85caa7b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Jasikowski?= Date: Thu, 26 Sep 2024 15:12:07 +0200 Subject: [PATCH 4/5] Linting fixes --- lib/ExpensiMark.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/ExpensiMark.ts b/lib/ExpensiMark.ts index 74299cad..8032f8e1 100644 --- a/lib/ExpensiMark.ts +++ b/lib/ExpensiMark.ts @@ -1,5 +1,3 @@ -import str from "./str"; - 'worklet'; import Str from './str'; @@ -82,7 +80,6 @@ const MARKDOWN_VIDEO_REGEX = new RegExp( const SLACK_SPAN_NEW_LINE_TAG = ''; export default class ExpensiMark { - getAttributeCache = (extras?: Extras) => { if (!extras) { return {attrCachingFn: undefined, attrCache: undefined}; @@ -90,7 +87,7 @@ export default class ExpensiMark { return { attrCachingFn: extras.mediaAttributeCachingFn ?? extras.cacheVideoAttributes, - attrCache: extras.mediaAttributeCache ?? extras.videoAttributeCache + attrCache: extras.mediaAttributeCache ?? extras.videoAttributeCache, }; }; @@ -732,7 +729,7 @@ export default class ExpensiMark { return `![${altText}](${imgSource})`; } return `!(${imgSource})`; - } + }, }, { From e0f9c4a70c566e56dd928dea955be18451c317f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Jasikowski?= Date: Thu, 26 Sep 2024 20:02:39 +0200 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com> --- lib/ExpensiMark.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/ExpensiMark.ts b/lib/ExpensiMark.ts index 8032f8e1..66fd1431 100644 --- a/lib/ExpensiMark.ts +++ b/lib/ExpensiMark.ts @@ -9,20 +9,24 @@ import * as Utils from './utils'; type Extras = { reportIDToName?: Record; accountIDToName?: Record; + /** * @deprecated Replaced with mediaAttributeCachingFn */ cacheVideoAttributes?: (vidSource: string, attrs: string) => void; + /** * @deprecated Replaced with mediaAttributeCache */ videoAttributeCache?: Record; + /** * Function used to cache HTML tag attributes during conversion to and from Markdown * @param mediaSource URI to media source * @param attrs Additional attributes to be cached */ mediaAttributeCachingFn?: (mediaSource: string, attrs: string) => void; + /** * Key/value cache for HTML tag attributes, where the key is media source URI, value is cached attributes */ @@ -700,7 +704,7 @@ export default class ExpensiMark { regex: /<]*src\s*=\s*(['"])(.*?)\1(.*?)>(?![^<][\s\S]*?(<\/pre>|<\/code>))/gi, /** * @param extras - The extras object - * @param match - The full match + * @param _match - The full match * @param _g1 - The first capture group (the quote) * @param imgSource - The second capture group - src attribute value * @param imgAttrs - The third capture group - any attributes after src