From bae8f19525b0f37344623ffccedcdc620bc4e839 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Wed, 18 May 2022 13:25:14 -0400 Subject: [PATCH] improve legacy shortcode handling (#1349) * add support for closing shortcodes * allow Shortcode.regex name param to be a regex * use Shortcode.regex for legacy shortcode matching * allow dashes in shortcode param names --- .../ckeditor/plugins/LegacyShortcodes.test.ts | 34 ++++++++-- .../lib/ckeditor/plugins/LegacyShortcodes.ts | 55 ++++++++-------- static/js/lib/ckeditor/plugins/util.test.ts | 63 ++++++++++++++++--- static/js/lib/ckeditor/plugins/util.ts | 33 +++++++--- 4 files changed, 135 insertions(+), 50 deletions(-) diff --git a/static/js/lib/ckeditor/plugins/LegacyShortcodes.test.ts b/static/js/lib/ckeditor/plugins/LegacyShortcodes.test.ts index d6e57b1a7..7783c8ec7 100644 --- a/static/js/lib/ckeditor/plugins/LegacyShortcodes.test.ts +++ b/static/js/lib/ckeditor/plugins/LegacyShortcodes.test.ts @@ -5,7 +5,7 @@ import LegacyShortcodes from "./LegacyShortcodes" import { LEGACY_SHORTCODES } from "./constants" import Paragraph from "@ckeditor/ckeditor5-paragraph/src/paragraph" -const quizTestMD = `{{< quiz_multiple_choice questionId="Q1_div" >}}{{< quiz_choices >}}{{< quiz_choice isCorrect="false" >}}sound, satisfiable{{< /quiz_choice >}}{{< quiz_choice isCorrect="false" >}}valid, satisfiable{{< /quiz_choice >}}{{< quiz_choice isCorrect="true" >}}sound, valid{{< /quiz_choice >}}{{< quiz_choice isCorrect="false" >}}valid, true{{< /quiz_choice >}}{{< /quiz_choices >}}{{< quiz_solution / >}}{{< /quiz_multiple_choice >}}` +const quizTestMD = `{{< quiz_multiple_choice questionId="Q1_div" >}}{{< quiz_choices >}}{{< quiz_choice isCorrect="false" >}}sound, satisfiable{{< /quiz_choice >}}{{< quiz_choice isCorrect="false" >}}valid, satisfiable{{< /quiz_choice >}}{{< quiz_choice isCorrect="true" >}}sound, valid{{< /quiz_choice >}}{{< quiz_choice isCorrect="false" >}}valid, true{{< /quiz_choice >}}{{< /quiz_choices >}}{{< quiz_solution >}}{{< /quiz_multiple_choice >}}` const getEditor = createTestEditor([Paragraph, LegacyShortcodes, Markdown]) @@ -31,20 +31,42 @@ describe("ResourceEmbed plugin", () => { test.each(LEGACY_SHORTCODES)( "should take in and return %p closing shortcode", async shortcode => { - const md = `{{< /${shortcode} >}}` + const md1 = `{{< /${shortcode} >}}` + const editor1 = await getEditor(md1) + // @ts-ignore + expect(editor1.getData()).toBe(md1) + + /** + * Hugo documentation uses the first version so it should be preferred. + * But Hugo accepts this version and image-gallery uses it. + * The editor will normalize it to the first version. + */ + const md2 = `{{}}` + const editor2 = await getEditor(md2) + // @ts-ignore + expect(editor2.getData()).toBe(md1) + } + ) + + test.each(LEGACY_SHORTCODES)( + "should take in and return %p shortcode with positional parameters", + async shortcode => { + const md = `{{< ${shortcode} arguments "and chemistry H{{< sub 2 >}}0" >}}` + const expected = `{{< ${shortcode} "arguments" "and chemistry H{{< sub 2 >}}0" >}}` const editor = await getEditor(md) // @ts-ignore - expect(editor.getData()).toBe(md) + expect(editor.getData()).toBe(expected) } ) test.each(LEGACY_SHORTCODES)( - "should take in and return %p shortcode with arguments", + "should take in and return %p shortcode with named parameters", async shortcode => { - const md = `{{< ${shortcode} arguments foo=123 html= >}}` + const md = `{{< ${shortcode} foo=123 html="" >}}` + const expected = `{{< ${shortcode} foo="123" html="" >}}` const editor = await getEditor(md) // @ts-ignore - expect(editor.getData()).toBe(md) + expect(editor.getData()).toBe(expected) } ) diff --git a/static/js/lib/ckeditor/plugins/LegacyShortcodes.ts b/static/js/lib/ckeditor/plugins/LegacyShortcodes.ts index 6164629e6..67bd0f233 100644 --- a/static/js/lib/ckeditor/plugins/LegacyShortcodes.ts +++ b/static/js/lib/ckeditor/plugins/LegacyShortcodes.ts @@ -5,6 +5,7 @@ import { editor } from "@ckeditor/ckeditor5-core" import MarkdownSyntaxPlugin from "./MarkdownSyntaxPlugin" import { TurndownRule } from "../../../types/ckeditor_markdown" import { LEGACY_SHORTCODES } from "./constants" +import { Shortcode } from "./util" const shortcodeClass = (shortcode: string) => `legacy-shortcode-${shortcode}` @@ -18,33 +19,35 @@ class LegacyShortcodeSyntax extends MarkdownSyntaxPlugin { get showdownExtension() { return function legacyShortcodeExtension(): ShowdownExtension[] { - return LEGACY_SHORTCODES.map( - (shortcode: string): ShowdownExtension => { - const shortcodeRegex = new RegExp(`{{< /?${shortcode} (.*?)>}}`, "g") - const closingShortcodeRegex = new RegExp( - `{{< /${shortcode} (.*?)>}}`, - "g" - ) - - return { - type: "lang", - regex: shortcodeRegex, - replace: (stringMatch: string, shortcodeArgs: string | null) => { - const isClosing = JSON.stringify( - closingShortcodeRegex.test(stringMatch) - ) - - const tag = `` - - return tag - } + const nameRegex = new RegExp(LEGACY_SHORTCODES.join("|")) + return [ + { + type: "lang", + /** + * It's important that there's a single regex for all the legacy + * shortcodes, rather than one per shortcode. Otherwise the order of + * the replacements is important. + * + * For example, image-gallery-item and sub are both legacy shortcodes + * and sometimes they are used together: + * {{< image-gallery-item ... "H{{< sub 2 >}}O" >}} + * If separate regexes are used, then image-gallery-item would need to + * come before sub so that the sub-replacement is not used on the above + * example. + */ + regex: Shortcode.regex(nameRegex, false), + replace: (stringMatch: string) => { + const shortcode = Shortcode.fromString(stringMatch) + const { isClosing } = shortcode + const params = shortcode.params.map(p => p.toHugo()).join(" ") + const tag = `` + + return tag } } - ) + ] } } @@ -64,7 +67,7 @@ class LegacyShortcodeSyntax extends MarkdownSyntaxPlugin { return `{{< ${isClosingTag ? "/" : ""}${shortcode} ${ rawShortcodeArgs !== undefined && rawShortcodeArgs !== null ? - decodeURIComponent(rawShortcodeArgs) : + `${decodeURIComponent(rawShortcodeArgs)} ` : "" }>}}` } diff --git a/static/js/lib/ckeditor/plugins/util.test.ts b/static/js/lib/ckeditor/plugins/util.test.ts index 57cee29d3..cafd14b11 100644 --- a/static/js/lib/ckeditor/plugins/util.test.ts +++ b/static/js/lib/ckeditor/plugins/util.test.ts @@ -46,15 +46,15 @@ describe("unescapeStringQuotedWith", () => { }) describe("Shortcode", () => { - describe("Shortcode.parse", () => { + describe("Shortcode.fromString", () => { it("parses shortcodes with named params", () => { const text = - '{{< some_shortcode cool_arg="cats and dogs" href_uuid=uuid456 >}}' + '{{< some_shortcode param-with-dash="cats and dogs" with_underscore=uuid456 >}}' const result = Shortcode.fromString(text) const params = [ - { name: "cool_arg", value: "cats and dogs" }, - { name: "href_uuid", value: "uuid456" } + { name: "param-with-dash", value: "cats and dogs" }, + { name: "with_underscore", value: "uuid456" } ].map(makeParams) expect(result).toStrictEqual( new Shortcode("some_shortcode", params, false) @@ -165,6 +165,28 @@ describe("Shortcode", () => { ) }) + it.each([ + { + text: "{{< /some_shortcode >}}", + expected: new Shortcode("some_shortcode", [], false, true) + }, + { + text: "{{< / some_shortcode >}}", + expected: new Shortcode("some_shortcode", [], false, true) + }, + { + text: "{{}}", + expected: new Shortcode("some_shortcode", [], false, true) + }, + { + text: "{{% /some_shortcode %}}", + expected: new Shortcode("some_shortcode", [], true, true) + } + ])("parses closing shortcodes", ({ text, expected }) => { + const result = Shortcode.fromString(text) + expect(result).toStrictEqual(expected) + }) + it("does not allow mixing named and positional params", () => { const text = '{{< resource uuid123 href_uuid="uuid456" >}}' expect(() => Shortcode.fromString(text)).toThrow( @@ -367,21 +389,37 @@ describe("Shortcode", () => { ]) }) + it("captures closing and opening shortcodes", () => { + const regex = Shortcode.regex("some_shortcode", false) + const text = ` + a {{< some_shortcode xyz >}} b + c {{< /some_shortcode >}} d + e {{}} f + ` + const match = text.match(regex) + expect(match).toStrictEqual([ + "{{< some_shortcode xyz >}}", + "{{< /some_shortcode >}}", + "{{}}" + ]) + }) + it.each([ { - name: "some_shortcode", - text: "a {{< some_shortcode xyz >}} b {{< some_shortcode_two 123 >}}", - expected: "{{< some_shortcode xyz >}}" + name: "some_shortcode", + text: + "a {{< some_shortcode xyz >}} b {{< some_shortcode_two 123 >}} {{< /some_shortcode xyz >}}", + expected: ["{{< some_shortcode xyz >}}", "{{< /some_shortcode xyz >}}"] }, { name: "some_shortcode_two", text: "a {{< some_shortcode xyz >}} b {{< some_shortcode_two 123 >}}", - expected: "{{< some_shortcode_two 123 >}}" + expected: ["{{< some_shortcode_two 123 >}}"] } ])("captures shortcodes of specified name", ({ text, expected, name }) => { const regex = Shortcode.regex(name, false) const match = text.match(regex) - expect(match).toStrictEqual([expected]) + expect(match).toStrictEqual(expected) }) it("tolerates an odd number of escaped quotes", () => { @@ -390,5 +428,12 @@ describe("Shortcode", () => { const match = text.match(regex) expect(match).toStrictEqual(['{{< shortcode "cat \\" >}}" >}}']) }) + + it("can take a regex as name parameter", () => { + const regex = Shortcode.regex(/cat|dog/, false) + const text = "a {{< cat meow >}} {{< dog woof >}} {{< dragon roar >}} b" + const match = text.match(regex) + expect(match).toStrictEqual(["{{< cat meow >}}", "{{< dog woof >}}"]) + }) }) }) diff --git a/static/js/lib/ckeditor/plugins/util.ts b/static/js/lib/ckeditor/plugins/util.ts index c70d36176..2bd54f5ab 100644 --- a/static/js/lib/ckeditor/plugins/util.ts +++ b/static/js/lib/ckeditor/plugins/util.ts @@ -102,14 +102,18 @@ export class Shortcode { isPercentDelimited: boolean + isClosing: boolean + constructor( name: string, params: ShortcodeParam[], - isPercentDelimited = false + isPercentDelimited = false, + isClosing = false ) { this.name = name this.params = params this.isPercentDelimited = isPercentDelimited + this.isClosing = isClosing const hasPositionalParams = params.some(p => p.name === undefined) const hasNamedParams = params.some(p => p.name !== undefined) @@ -127,19 +131,22 @@ export class Shortcode { */ toHugo() { const stringifiedArgs = this.params.map(p => p.toHugo()) - const interior = [this.name, ...stringifiedArgs].join(" ") + const name = this.isClosing ? `/${this.name}` : this.name + const interior = [name, ...stringifiedArgs].join(" ") if (this.isPercentDelimited) { return `{{% ${interior} %}}` } return `{{< ${interior} >}}` } + private static IS_CLOSING_REGEXP = /\s*(?\/)?\s*/ + /** * Regexp used for matching individual shortcode arguments. */ private static ARG_REGEXP = new RegExp( [ - /((?[a-zA-Z_]+)=)?/.source, + /((?[a-zA-Z\-_]+)=)?/.source, "(", /("(?.*?)(? { return new ShortcodeParam( @@ -167,7 +180,7 @@ export class Shortcode { ) }) - return new Shortcode(name, params, isPercentDelmited) + return new Shortcode(name, params, isPercentDelmited, isClosing) } /** @@ -231,13 +244,15 @@ export class Shortcode { * Returns a global regex that matches shortcodes of given name. Useful for * use in Showdown rules. */ - static regex(name: string, isPercentDelimited: boolean) { + static regex(name: string | RegExp, isPercentDelimited: boolean) { const opener = isPercentDelimited ? "{{%" : "{{<" const closer = isPercentDelimited ? "%}}" : ">}}" const regex = new RegExp( [ opener, - String.raw`\s${name}\s`, + Shortcode.IS_CLOSING_REGEXP.source, + name instanceof RegExp ? `(${name.source})` : name, + String.raw`\s`, /** * Non-greedily capture anything up until the closer. Except if there is * a non-escaped quotation mark, then there must be another.