fix: keep pipe tables visible in commonmark - #729
Conversation
eszlamczyk
left a comment
There was a problem hiding this comment.
Thanks @huytdps13400 for your pull request!
While this change could work, I believe there is a cleaner way to do it. Since tables are part of the strict GFM specification, gating them behind their own dedicated tables flag threaded through the whole stack is unnecessary - and it leaves the mode half-done, since strikethrough and task lists are still parsed unconditionally, so flavor="commonmark" isn't really CommonMark.
Instead of putting a flavor-derived value inside the Md4cFlags struct, I'd rather thread the information about the flavor being GFM through to the parser and resolve the extension flags there - not just tables, but the whole GFM set (strikethrough, task lists) in one place:
std::shared_ptr<MarkdownASTNode> MD4CParser::parse(
const std::string &markdown, const Md4cFlags &md4cFlags, bool isGFM) {
unsigned flags = MD_FLAG_NOHTML | MD_FLAG_SPOILERS;
if (isGFM) {
flags |= MD_FLAG_TABLES | MD_FLAG_STRIKETHROUGH | MD_FLAG_TASKLISTS;
}
// ...On the JS side this stays what you already have, just as its own signal rather than member of md4cFlags, e.g. isGFM: flavor === 'github'. On iOS and Android you might need to therad this a bit, confirm that you need or reuse already existing infrastructure.
When you're done please tag me here and I'll re-review. Please address my other review comments as well.
|
@eszlamczyk Addressed the review in |
There was a problem hiding this comment.
LGTM - the core change is exactly the shape I wanted: a single isGFM signal gating the whole GFM extension set (tables + strikethrough + task lists) in the parser, driven off flavor === 'github' at the RN layer, with no tables member left in Md4cFlags. Threading through the RN iOS and Android paths (including measurement and streaming) is thorough.
One small nitpick, which I'll take care of myself so we don't hold this up: the standalone native packages (enriched-markdown-ios, android-enriched-markdown) don't render GFM yet - there's no table/task-list/strikethrough renderer in them - so exposing an isGFM toggle on their public parse API (and documenting it in the iOS README) advertises something they can't do. I'll drop the isGFM parameter from those two packages and just pin CommonMark at their bridge call, and remove the GFM mention from the iOS README. That keeps GFM entirely a concern of the RN layer until the dedicated iOS GFM rendering work lands. Thanks for the solid rework here!
What/Why?
flavor="commonmark"used the single-TextView renderer, but the shared MD4C parser still enabled the GFM tables extension. A table-shaped string therefore became aTableAST node that the CommonMark renderer cannot represent, so the entire message rendered as an empty view.This change makes table parsing an internal flavor policy:
Plain prose and shell pipelines containing a single
|already parsed as text and remain unchanged. Spoiler parsing (||text||) also remains enabled and unchanged.Closes #645.
Testing
yarn lintyarn typecheckyarn test --runInBand— 19/19 passedyarn prepare— TypeScript, module build, and native Codegen passedxcodebuild test— 274/274 passedtestDebugUnitTest,ktlintCheck,compileDebugAndroidTestKotlin, andassembleRelease— passedcommonmark_pipe_table_test.yamlon API 35 — passed; the preview exposed| Model | Speed | |---|---| | Claude | Fast |lint,typecheck,clang-format,ktlint) — passedPR Checklist