fix(mdx): issues with html in callout titles for mdx rendering#1384
fix(mdx): issues with html in callout titles for mdx rendering#1384rafegoldberg merged 5 commits intonextfrom
Conversation
|
want to just flag this but we do have another option on how to fix this, it was my first attempt in f0ae90c which basically now allows the |
| node.children[0].position.start.offset += match.length; | ||
| node.children[0].position.start.column += match.length; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
this is the regular rendering pipeline like when no block contents
There was a problem hiding this comment.
Wait we should leave the mdast tests untouched as we do still want them. We should add on tests instead for the mdxishMdast
There was a problem hiding this comment.
oh yea i just rearranged them
…dmeio/markdown into falco/callout-heading-issues
kevinports
left a comment
There was a problem hiding this comment.
Had a few issues with the test updates, but this looks like it's working and I don't want to block merging it to fix the fire.
I agree flagging the transformer logic for mdxish is a safe call. But In the future I wonder if it'd be better to just add a new mdxish-specific callout transformer instead of using a shared transformer.
| import calloutTransformer from '../../processor/transform/callouts'; | ||
|
|
||
| const mdxishMdast = (md: string) => { | ||
| const processor = unified().use(remarkParse).use(remarkGfm).use(calloutTransformer, { isMdxish: true }); |
There was a problem hiding this comment.
Shouldn't this use mdxishAstProcessor instead of a new vanilla pipeline?
| it('parses a blockquote with text as the title', () => { | ||
| const md = '> 📘 > helo\n>\n> Hello.'; | ||
| const tree = mdast(md); | ||
| const tree = mdxishMdast(md); |
There was a problem hiding this comment.
I'd expect that we'd be forking these tests to assert that this renders correctly for BOTH mdast and mdxishMdast. We still want coverage on the mdx transformer.
| } | ||
|
|
||
| const defaultTransformers = [calloutTransformer, codeTabsTransformer, gemojiTransformer, embedTransformer]; | ||
| const defaultTransformers: PluggableList = [ |
There was a problem hiding this comment.
Why do we need PluggableList here?
There was a problem hiding this comment.
Fix works from my quick once over. But I'd second what @kevinports said:
…in the future I wonder if it'd be better to just add a new mdxish-specific callout transformer instead of using a shared transformer?
Feels like sharing it one-for-one is just a recipe for 🦶🔫ing ourselves at this point, ya know?
|
This PR was released!🚀 Changes included in v13.6.3 |
🧰 Changes
While doing some change to the MDXish pipeline over in #1362, we were also hitting the callout transformer which overflowed and was used by the MDX pipeline. The regression came in when rendering HTML in callout headings
This broke only for MDX because the mdast & compile pipeline calls
remarkMdxbefore it hits the callout transformer, soremarkMdxalready processed<div>World</div>to mdxJsxElement text nodes, and toMarkdown by default don't expect those nodes.This did not happen in MDXish because we never "preprocess"
<div>World</div>so when it came to the callout transformer, it was the pure unprocessed HTML string.Fix
Since the end goal is MDXish, I reckon it'd be better to just add a flag
isMdxishbehind all the title parsing logic that was introduced in #1362. this meant that MDX pipeline remains unaffected and rendering block level elements in the callout headings is specific to only MDXishNote
This does mean that some block level elements are rendered in MDXish and NOT in MDX. it looks like this was never even supported in MDX and the reason #1362 was done in the first place was to mimic legacy RDMD behaviour where they did render block level elements in callout headings
🧬 QA & Testing
In the demo app, test using all 3 rendering pipelines and confirm that having special syntax (bold, italic) works and block level elements work as well. Printing stuff like this should not error