Repository navigation
Conversation
## Description After importing a bare URL, changing its destination, label or formatting is lost on Markdown export. The saved literal style currently emits only the visible text. Keep the bare form only when its plain text still parses as the same link. Otherwise use the default link serializer. Unchanged literals keep their syntax. ## Changeset `.changeset/mdast-edited-autolink-literals.md` ## Test plan `pnpm run test-unit packages/lexical-mdast --maxWorkers=1` ### Before The added round-trip test with the original exporter: ```text Expected: "[https://one.example](https://changed.example) [caption](https://two.example) [**https://three.example**](https://three.example)" Received: "https://one.example caption https://three.example" ``` ### After ```text Test Files 8 passed (8) Tests 286 passed (286) ``` The regression reimports the export and checks destinations, labels, bold formatting and a second export. Browser suites were not run; this exercises editor state and serialization in the package's Node unit tests.
## Description After importing a bare URL, changing its destination, label or formatting is lost on Markdown export. The saved literal style currently emits only the visible text. Keep the bare form only when its plain text still parses as the same link. Otherwise use the default link serializer. Reuse parsed destinations during lookahead and repeated exports, retaining only the literal labels seen in the latest export. Unchanged literals keep their syntax. ## Changeset `.changeset/mdast-edited-autolink-literals.md` ## Test plan `pnpm run test-unit packages/lexical-mdast --maxWorkers=1` ### Before The added round-trip test with the original exporter: ```text Expected: "[https://one.example](https://changed.example) [caption](https://two.example) [**https://three.example**](https://three.example)" Received: "https://one.example caption https://three.example" ``` ### After ```text Test Files 8 passed (8) Tests 287 passed (287) ``` The tests cover edited-link round trips and parser calls during lookahead and repeated exports. `pnpm run ci-check`, package builds and the full unit suite with fixture-only `GIT_CONFIG_GLOBAL=/dev/null` also pass. Browser suites were not run; this exercises editor state and serialization in the package's Node unit tests.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
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://avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks, this looks good. Edited bare URLs now survive a Markdown round trip, and the new round-trip test in MdastImportExport.test.ts checks both export and re-import. Everything below is optional and non-blocking.
Optional suggestions
- The bare-form check doesn't look at neighbouring text (
packages/lexical-mdast/src/MdastExport.ts:207,:674-700). The GFM spec's extended-autolink rule recognises a literal only at the start of a line, after whitespace, or after*,_,~or(, and the literal runs on over the characters that follow it. The label is parsed on its own, so deleting the space insee https://one.example xstill exportsseehttps://one.example x, which re-imports with no link. Typing/abcstraight after the link changes the re-imported URL. Both cases behaved the same before this PR, so a follow-up is fine. The handler'sinfo.beforeandinfo.afterwould let it fall back to[label](url)there. Meanwhile, the comment "only while it still parses as the same link" could mention the limitation. - The validation cache keeps only the latest export (
MdastExport.ts:722). Each uncached label runs a fullfromMarkdown, roughly 0.4 ms. On a document with 300 distinct bare URLs the first export after import went from about 18 ms to about 139 ms. Calling$convertToMarkdownString(node)on 20 different paragraphs in turn went from about 1.8 ms to about 22 ms, because each call evicts the other entries. Repeated whole-document exports are unchanged. A label's validity doesn't depend on the export root, so a bounded map that isn't cleared on every call would avoid the repeat cost. - Copy-as-Markdown of part of a bare URL now gives
[https://one.ex](https://one.example)rather thanhttps://one.ex. That is consistent with the fix, since the old output re-read as a link to the truncated URL. It is still an observable output change: a test would pin it, and AGENTS.md asks for the changeset'sVerify:paragraph to describe output changes, so it could mention it. - One parametrised case for edited
www.and email literals, and for a literal inside a GFM table cell, would pin behaviour that already works with this change.
| node.children.length === 1 && | ||
| node.children[0].type === 'text' | ||
| ) { | ||
| // Preserve the bare form only while it still parses as the same link. |
There was a problem hiding this comment.
Optional, non-blocking: this check only sees the label, so neighbouring text can still change how the bare form re-parses. GFM extended autolinks need a line start, whitespace, or */_/~/( before them, and they run on over the characters that follow. For example, deleting the space in see https://one.example x exports seehttps://one.example x, which re-imports without a link, the same as before this PR. A follow-up could use info.before/info.after to fall back to [label](url) in those cases; until then, this comment could note the limitation.
There was a problem hiding this comment.
Added a note on the neighbouring-text limitation in that comment; the behavior there is unchanged in this PR.
| syntaxToMarkdown(getLiteralUrl, collectHardBreaks(tree)), | ||
| ], | ||
| }); | ||
| literalUrls = nextLiteralUrls; |
There was a problem hiding this comment.
Optional, non-blocking: replacing the cache on every call means that alternating $convertToMarkdownString(node) calls on different subtrees, or a selection export between document exports, re-parse every literal each time (roughly 0.4 ms per label). A label's validity doesn't depend on the export root, so a bounded map that isn't cleared on every call would keep the benefit.
There was a problem hiding this comment.
The cache now keeps up to 1,000 labels per exporter (oldest evicted) instead of being replaced on every call, with a regression for alternating subtree and selection exports.
## Description
Alternating subtree exports or a selection export between document exports
replaces cached literal results and repeats parsing. Keep up to 1,000 labels
per exporter, evicting the oldest instead of replacing the map after each
export. Note the existing neighbouring-text limitation without changing
literal serialization rules.
## Test plan
### Before
`pnpm run test-unit packages/lexical-mdast/src/__tests__/unit/MdastImportExport.test.ts --maxWorkers=1`
with the new subtree/selection test and the previous implementation:
```text
AssertionError: expected 5 to be 2 // Object.is equality
Test Files 1 failed (1)
Tests 1 failed | 106 passed (107)
```
### After
`pnpm run test-unit packages/lexical-mdast --maxWorkers=1`
```text
Test Files 8 passed (8)
Tests 288 passed (288)
```
`pnpm run ci-check`, development/production/www builds and declaration
validation pass. The full unit suite with fixture-only
`GIT_CONFIG_GLOBAL=/dev/null` passes: 31,522 passed, 470 expected fail,
135 skipped. Browser suites were not rerun locally.
There was a problem hiding this comment.
Thanks, this looks good. Re-parsing the literal's label with the editor's own grammar and falling back to [label](url) is a clean fix: edited destinations, labels and formatting now survive a round trip, unchanged bare URLs keep their spelling, and an edited label is escaped instead of being written raw, so typed [x](...) or <b> no longer comes back as live syntax. The new tests fail on main with exactly the loss the description shows and pass here.
A few optional, non-blocking suggestions:
- Cache eviction (
packages/lexical-mdast/src/MdastExport.ts, lines 693-696). The 1,000-entry cap evicts the oldest insert while lookups walk the document in order. Once a document has more than 1,000 distinct literal labels every lookup misses, so every export re-parses all of them (roughly 10x themainexport time at 1,500 links in a local Node run). Below the cap, each run of newly typed labels eventually evicts the oldest document label and that export re-parses the whole document. Keeping the labels the latest whole-document export used (as the earlier revision of this PR did) and letting subtree and selection exports only add would bound memory by the document without either cliff. - Text typed right after a literal (pre-existing, already noted in the comment at line 209).
see https://a.examplefollowed by a typedxstill exportssee https://a.examplex, which GFM reads as a single link ("After a valid domain, zero or more non-space non-<characters may follow"). The handler'sinfo.aftercould gate the bare form; this is fine as a follow-up. - PR description. The Test plan's After block (287 tests) and the sentence about retaining only the labels from the latest export describe the previous commit; the suite now has 288 tests and the cache is shared across exports. AGENTS.md (Commits and Pull Requests) asks the Test plan to carry the real output of the command that was run.
| literalUrls.set(text, url); | ||
| if (literalUrls.size > 1000) { | ||
| literalUrls.delete(literalUrls.keys().next().value!); | ||
| } |
There was a problem hiding this comment.
Optional, non-blocking: lookups walk the document in order while this evicts the oldest insert, so once a document has more than 1,000 distinct literal labels every lookup misses and every export re-parses all of them (about 512 ms against 51 ms on main at 1,500 links in a local Node run). Below the cap, enough newly typed labels evict the oldest document label and that export re-parses everything again. Keeping the labels the latest whole-document export used, and letting subtree and selection exports only add, would avoid both.
There was a problem hiding this comment.
Done: a whole-document export now replaces the remembered labels with the ones it used, and subtree and selection exports only add to them, so there is no eviction. A new test checks that a second export of a 1,001-link document parses nothing again (it re-parsed all 1,001 labels before). A warm export of 1,500 bare URLs went from about 190 ms on the previous commit to about 6 ms, the same as main.
| ) { | ||
| // Preserve the bare form only while it still parses as the same link. | ||
| // Edits to its destination, label or formatting need resource syntax. | ||
| // Neighbouring text can still change how the bare form re-parses. |
There was a problem hiding this comment.
Optional follow-up: this case is reachable by typing. see https://a.example plus a typed x (which lands outside the link) exports see https://a.examplex and re-imports as a link to https://a.examplex, since GFM lets any non-space non-< characters follow a valid domain. Checking info.after here before returning the bare text would close it.
There was a problem hiding this comment.
I left this one out. In the link handler info.after holds only the first character of what follows, so a check cannot tell a sentence-ending . (which GFM leaves outside the autolink) from .x. Guarding on any following non-space character would turn every imported https://a.example. into [https://a.example](https://a.example). on an unchanged round trip. Fixing it properly needs the full serialized suffix, so it fits better as a separate change.
There was a problem hiding this comment.
Thanks, this looks good. Re-parsing the literal's label with the editor's own grammar and falling back to [label](url) fixes the lost edits cleanly: changed destinations, labels and formatting now survive a round trip, unchanged bare URLs, www. addresses and emails keep their spelling, and an edited label is escaped rather than written raw, so text like [x](y) typed into a label no longer comes back as a live link. The cache change in the latest commit makes repeated exports cost the same as on main. Everything below is optional and non-blocking.
Optional suggestions
- First export of a link-heavy document (
packages/lexical-mdast/src/MdastExport.ts:684). Every label not yet cached costs a fullfromMarkdownrun, about 0.35 ms each in a local Node run: the first export after import went from about 11 ms to 111 ms with 300 bare URLs, and from 60 ms to 544 ms with 1,500; second exports are unchanged. Headless one-shot conversions (import, then export once) always take that path. Skipping the parse while a literal's label and URL are still the ones it was imported with, or parsing the uncached labels of one export in a single batch, would remove most of it. A follow-up is fine. - The
Verify:paragraph (.changeset/mdast-edited-autolink-literals.md:7). AGENTS.md (Commits and Pull Requests) asksVerify:to say what to test when output changes, and three outputs change besides edited literals: a literal inside emphasis or strikethrough (**https://a.example**) now exports as[**https://a.example**](https://a.example)instead of dropping the formatting; an editor withoutMdastAutolinkLiteralExtensionwrites<https://a.example>for literal-styled links loaded from JSON; and copying part of a bare URL as Markdown gives[https://one.ex](https://one.example). ContributedmdastExtensionstransforms also run during export now, once per uncached label, which extension authors may want to know. - Tests for those three outputs in
MdastImportExport.test.tswould keep them from drifting. An edited email literal is also not covered yet. - Neighbouring text, already noted in the comment at
MdastExport.ts:208-210and agreed as a separate change. Your reason for leavinginfo.afterout holds. GFM's preceding-character rule looks at exactly one character, soinfo.beforecould handle the deleted-space case without that ambiguity when you pick this up.
| const getLiteralUrl = (text: string): string | null => { | ||
| let url = literalUrls.get(text); | ||
| if (url === undefined) { | ||
| const parsed = fromMarkdown(text, { |
There was a problem hiding this comment.
Optional, non-blocking: each label not yet cached costs a full fromMarkdown run here, about 0.35 ms in a local Node run. The first export after import went from about 11 ms to 111 ms with 300 bare URLs, and from 60 ms to 544 ms with 1,500; repeated exports match main. Headless one-shot conversions always pay it. Skipping the parse while the label and URL are still the imported ones, or batching the uncached labels of one export into a single parse, would remove most of it. Fine as a follow-up.
|
|
||
| [fix] Preserve edits to imported autolink literals when exporting Markdown | ||
|
|
||
| Verify: Import a bare URL with `MdastAutolinkLiteralExtension`, change its destination, label or text formatting, then export and reimport it. The edits should survive. Unchanged bare URLs and email addresses should keep their original syntax. |
There was a problem hiding this comment.
Optional, non-blocking: AGENTS.md asks Verify: to say what to test when output changes. Besides edited literals, three outputs change: **https://a.example** now exports as [**https://a.example**](https://a.example) (the formatting used to be dropped), an editor without MdastAutolinkLiteralExtension writes <https://a.example> for literal-styled links loaded from JSON, and copying part of a bare URL gives [https://one.ex](https://one.example). One sentence naming them would tell upgraders where to look.
Description
After importing a bare URL, changing its destination, label or formatting is lost on Markdown export. The saved literal style currently emits only the visible text.
Keep the bare form only when its plain text still parses as the same link. Otherwise use the default link serializer. Cache the labels used by the latest whole-document export; subtree and selection exports only add to that cache. Unchanged literals keep their syntax; neighbouring-text ambiguity remains unchanged.
Changeset
.changeset/mdast-edited-autolink-literals.mdTest plan
pnpm run test-unit packages/lexical-mdast --maxWorkers=1Before
The added round-trip test with the original exporter:
After
The tests cover edited-link round trips and parser calls during lookahead, repeated, subtree and selection exports, including a 1,001-link document whose second export reuses every label.
pnpm run ci-checkpasses. Browser suites were not run; these are editor-state and serialization tests in the package's Node unit tests.