Repository navigation
[lexical-markdown] Feature: Export option to keep ambiguous < and & as the author typed them - #9722
potatowagon wants to merge 2 commits into
Conversation
…&` as the author typed them ## Description Since v0.53.0 (#9481) the exporter writes a literal `<` or `&` that could read as raw HTML or as a character reference as a numeric character reference, so that re-importing the Markdown gives back the text the editor held. That is the right default, but it assumes the Markdown is read back by Lexical. An app whose renderer is meant to read the inline HTML and the character references an author writes gets `<u>under</u>` and `&#36;5` on the page instead; several surfaces inside Meta render exactly that way and have no knob to ask for the pre-v0.53.0 output. `$convertToMarkdownString` and `$convertSelectionToMarkdownString` take a `MarkdownExportOptions` object as their last argument: ```ts $convertToMarkdownString(TRANSFORMERS, undefined, false, { escapeAmbiguousHtml: false, }); ``` The default is unchanged. Brackets that would start a link, an image, a link reference definition or a check list item are still escaped, since nothing else keeps that text readable, and the format characters keep their backslash escapes. The option rides on the context the export already carries, so no signature below the entry points changes. ## Changeset `.changeset/markdown-escape-ambiguous-html-option.md`, `[feature]` on `@lexical/markdown`. ## Test plan New cases in `MarkdownEscaping.test.ts` cover the option across single and split text nodes, the selection export, what it still escapes, and the default. ### Before ``` $ pnpm exec vitest run --project unit packages/lexical-markdown/src/__tests__/unit/MarkdownEscaping.test.ts Test Files 1 passed (1) Tests 169 passed (169) ``` (the option did not exist) ### After ``` $ pnpm exec vitest run --project unit packages/lexical-markdown/src/__tests__/unit/MarkdownEscaping.test.ts Test Files 1 passed (1) Tests 199 passed (199) $ pnpm exec vitest run --project unit packages/lexical-markdown Test Files 9 passed (9) Tests 944 passed (944) $ pnpm run tsc (exit 0) $ pnpm run flow Found 0 errors $ pnpm exec prettier --check <changed files> All matched files use Prettier code style! $ pnpm exec eslint <changed files> (no findings) ``` `pnpm run ci-check` as a whole was not run here: this environment installs with `--ignore-scripts`, so `@lexical/devtools` has no wxt-generated types and its `tsc --noEmit` fails on `browser` and `defineBackground` regardless of this change.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
lexical
@lexical/a11y
@lexical/clipboard
@lexical/code
@lexical/code-core
@lexical/code-prism
@lexical/code-shiki
@lexical/compiler
@lexical/devtools-core
@lexical/dragon
@lexical/eslint-plugin
@lexical/extension
@lexical/fast-check
@lexical/file
@lexical/hashtag
@lexical/headless
@lexical/history
@lexical/html
@lexical/internal
@lexical/link
@lexical/list
@lexical/mark
@lexical/markdown
@lexical/mdast
@lexical/offset
@lexical/overflow
@lexical/plain-text
@lexical/react
@lexical/rich-text
@lexical/selection
@lexical/table
@lexical/tailwind
@lexical/text
@lexical/utils
@lexical/y
@lexical/yjs
commit: |
![meta-code-review[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks, this looks good. The option is additive, the default output is unchanged, and the new cases in MarkdownEscaping.test.ts fail without the change and pass with it. The suggestions below are optional and non-blocking.
- Nested exports lose the option (
packages/lexical-markdown/src/MarkdownExport.ts:998-999).withBracketContextbuilds a new context from its ownoptions. A transformer that exports nested content through$convertToMarkdownStringhas no way to pass them, becauseElementTransformer.exportdoes not receive options. The playground'sTABLEtransformer does exactly this for each cell (packages/lexical-playground/src/plugins/MarkdownTransformers/index.ts:226). So with{escapeAmbiguousHtml: false}, paragraphs come out as typed while table cells are still escaped (| <u>x</u> |). Inheriting from the enclosing export whenoptionsis undefined would cover it:A test with a transformer that makes a nested call would pin it.escapeAmbiguousHtml: options === undefined ? previous === null || previous.escapeAmbiguousHtml : options.escapeAmbiguousHtml !== false,
- Docs caveats (
packages/lexical-markdown/README.md:102-107and the JSDoc atMarkdownExport.ts:43-53). CONTRIBUTING.md asks for documentation with API changes, and three caveats would help readers:- the renderer is responsible for sanitizing, since anything typed or pasted as plain text now reaches it as raw HTML;
- the output is not meant to be read back with
$convertFromMarkdownString, which decodes numeric references ($5re-imports as$5); - the option applies only when
shouldPreserveNewLinesisfalse, because that other mode never escapes<,&or brackets.
| escapeAmbiguousHtml: | ||
| options === undefined || options.escapeAmbiguousHtml !== false, |
There was a problem hiding this comment.
Optional: a transformer that exports nested content through $convertToMarkdownString (the playground's TABLE does this per cell) lands here with options === undefined, so table cells keep escaping while the rest of the document is written as typed. Inheriting from the enclosing export would cover it:
escapeAmbiguousHtml:
options === undefined
? previous === null || previous.escapeAmbiguousHtml
: options.escapeAmbiguousHtml !== false,There was a problem hiding this comment.
Done in daf2cda — a nested export now inherits the option from the export it runs inside, with your spelling of the default. A new case in MarkdownEscaping.test.ts exports a cell-like container through a second $convertToMarkdownString and asserts both directions: | <u>under</u> | with the option, | <u>under</u> | without it.
| `$convertToMarkdownString` and `$convertSelectionToMarkdownString` take a | ||
| `MarkdownExportOptions` object as their last argument. Passing | ||
| `{escapeAmbiguousHtml: false}` writes `<` and `&` as the author typed them, for | ||
| an app whose renderer is meant to read the inline HTML and the character | ||
| references written in the editor; brackets and format characters are escaped | ||
| either way. |
There was a problem hiding this comment.
Optional: it would help to add that (1) the renderer must sanitize, since typed or pasted text now reaches it as raw HTML; (2) this output is not meant for $convertFromMarkdownString, which decodes numeric references ($5 re-imports as $5); and (3) the option applies only when shouldPreserveNewLines is false.
There was a problem hiding this comment.
Done in daf2cda — the README paragraph now names all three: the renderer has to sanitize, since text typed or pasted into the editor reaches it as raw HTML; the output is not meant for $convertFromMarkdownString, which decodes numeric references, so $5 re-imports as $5; and the option only applies to the default serializer, since shouldPreserveNewLines escapes nothing beyond the format characters. It also records that a nested export inherits the option.
…ort, and document what the option costs ## Description Review follow-ups on the `escapeAmbiguousHtml` option. A transformer that exports nested content calls `$convertToMarkdownString` again without options — the playground's table does this per cell — so that export opened a context of its own and escaped, while the document around it was written as typed. A nested export now inherits the option from the export it runs inside; a top-level export with no options still escapes. The README says what comes with the option: the renderer has to sanitize, since text typed or pasted into the editor reaches it as raw HTML; the output is not meant for `$convertFromMarkdownString`, which decodes numeric references, so `$5` re-imports as `$5`; and it applies only to the default serializer, since `shouldPreserveNewLines` escapes nothing beyond the format characters. ## Test plan A new case exports a cell-like container through a second `$convertToMarkdownString`, and asserts the nested content both ways: ``` $ pnpm exec vitest run --project unit packages/lexical-markdown Test Files 9 passed (9) Tests 945 passed (945) $ pnpm run tsc (exit 0) $ pnpm run flow Found 0 errors $ pnpm exec prettier --check <changed files> All matched files use Prettier code style! $ pnpm exec eslint <changed files> (no findings) ```
There was a problem hiding this comment.
Thanks, this looks good. The option is additive, every existing call keeps its output, and both earlier suggestions (nested exports inheriting the option, and the README caveats) are in, with a test that checks both directions. The suggestions below are optional and non-blocking.
{}andnulloptions (packages/lexical-markdown/src/MarkdownExport.ts:1001-1004). The option is inherited only whenoptionsisundefined. Inside an export run with{escapeAmbiguousHtml: false}, a nested$convertToMarkdownString(TRANSFORMERS, node, false, {})goes back to escaping (| <u>in</u> |). So a transformer that always forwards an options object loses the inheritance the README describes, and a JavaScript caller passingnullgets aTypeError. Deciding per field covers both:This is worth settling before the option ships, since changing whatescapeAmbiguousHtml: options == null || options.escapeAmbiguousHtml === undefined ? previous === null || previous.escapeAmbiguousHtml : options.escapeAmbiguousHtml !== false,
{}means later would change output.- Sanitizing caveat in the JSDoc (
MarkdownExport.ts:43-53). The README now says the renderer has to sanitize. TheescapeAmbiguousHtmlJSDoc, which is what editors show at the call site, doesn't say it, and one sentence there would carry it. - Test plan output. AGENTS.md ("Commits and Pull Requests") asks the Test plan to carry the real output of the command that was run. The pasted counts (169 before and 199 after for
MarkdownEscaping.test.ts, 944 for the package) predate the latest commit. On the current base the file has 179 tests; at this head it has 200, and the package has 945.
| escapeAmbiguousHtml: | ||
| options === undefined | ||
| ? previous === null || previous.escapeAmbiguousHtml | ||
| : options.escapeAmbiguousHtml !== false, |
There was a problem hiding this comment.
Optional: inheritance only happens when options is undefined, so a nested export passed {} (or {escapeAmbiguousHtml: undefined}) goes back to escaping inside an export run with the option off, and null throws a TypeError. Deciding per field covers both:
escapeAmbiguousHtml:
options == null || options.escapeAmbiguousHtml === undefined
? previous === null || previous.escapeAmbiguousHtml
: options.escapeAmbiguousHtml !== false,| * Set it to `false` when the export feeds a renderer that is meant to read | ||
| * the inline HTML and the character references an author writes, which is | ||
| * how the editor behaved before v0.53.0. The brackets that would start a | ||
| * link, an image, a link reference definition or a check list item are | ||
| * escaped either way, since nothing else keeps that text readable. |
There was a problem hiding this comment.
Optional: the README says the renderer has to sanitize when this is false; one sentence here would put that caveat where callers see it in their editor.
|
Closing this: the breakage it was written for is being solved the other way round, and that leaves the option without a consumer. #9600 narrowed the bracket escaping to the places a bracket would actually start Markdown syntax, which covers the citation text ( What is on the branch The sibling fix for the same sync, #9567, is unrelated to this and still open. |
Description
Since v0.53.0 (#9481) the exporter writes a literal
<or&that could readas raw HTML or as a character reference as a numeric character reference, so
that re-importing the Markdown gives back the text the editor held. That is
the right default, but it assumes the Markdown is read back by Lexical. An app
whose renderer is meant to read the inline HTML and the character references
an author writes gets
<u>under</u>and&#0036;5on the pageinstead; several surfaces inside Meta render exactly that way and have no
knob to ask for the pre-v0.53.0 output.
$convertToMarkdownStringand$convertSelectionToMarkdownStringtake aMarkdownExportOptionsobject as their last argument:The default is unchanged. Brackets that would start a link, an image, a link
reference definition or a check list item are still escaped, since nothing
else keeps that text readable, and the format characters keep their backslash
escapes. The option rides on the context the export already carries, so no
signature below the entry points changes.
Changeset
.changeset/markdown-escape-ambiguous-html-option.md,[feature]on@lexical/markdown.Test plan
New cases in
MarkdownEscaping.test.tscover the option across single andsplit text nodes, the selection export, what it still escapes, and the
default.
Before
(the option did not exist)
After
pnpm run ci-checkas a whole was not run here: this environment installswith
--ignore-scripts, so@lexical/devtoolshas no wxt-generated types andits
tsc --noEmitfails onbrowseranddefineBackgroundregardless ofthis change.