fix(backend) replace showdown (markdown) library#2711
Conversation
|
@klocke-io, @holgerkoser You have pull request review open invite, please check |
Maybe we will introduce them later, keep this PR simple for now
| } | ||
| const githubIssues = await searchIssues({ state: 'open', title }) | ||
| return _.map(githubIssues, fromIssue) | ||
| return Promise.all(_.map(githubIssues || [], fromIssue)) |
There was a problem hiding this comment.
did you encounter a case where adding the fallback || [] was necessary?
There was a problem hiding this comment.
Yes I think if the request failed it was undefined but not 100% sure
There was a problem hiding this comment.
then in line 134, do we also need the fallback out of the same reasons or does it behave differently and therefore not needed?
return Promise.all(_.map(githubComments, githubComment =>
There was a problem hiding this comment.
hm probably this error did never occur because if issues failed with error it will never try to load comments, but yes if this fails we should also have a fallback. I will add it
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: klocke-io, petersutter The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
LGTM label has been added. DetailsGit tree hash: 48acc871d2fbc72fbb12ca635401986b980239fa |
|
New changes are detected. LGTM label has been removed. |
|
/cherry-pick hotfix-1.83 |
|
@grolu: new pull request created: #2728 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Summary
This PR replaces the unmaintained
showdownMarkdown library with a modern, well-maintained Markdown processing pipeline based on unified / remark / rehype.The goal was to:
Considerations: Choosing a Markdown Library
I evaluated several Markdown libraries with the requirement that GitHub-style Markdown should be supported either natively or via plugins.
Evaluated candidates
markdown-itmarkedActively maintained and fast
Limited plugin ecosystem
Missing features such as emoji support
Would require manual handling for:
micromarkunified / remark / rehype(chosen)micromarkGiven the trade-offs, unified/remark/rehype was chosen as the best long-term solution.
Handling the CommonJS Test Base
All modern Markdown libraries (except
markdown-it) are ESM-only.Our test suite, however, is still CommonJS-based.
To solve this cleanly:
The actual Markdown implementation was moved to
markdown.engine.mjs(pure ESM)Rollup is configured to copy this file to
distwithout transpilingmarkdown.jsacts as a wrapper:markdown.cjsdynamically loads the engine viaawait import()The real
markdown.engine.mdis used by the markdown test suite onlyAll other indirect dependencies to the markdown engine have been replaced by a global mock
This allows:
Additionally:
markdown.spec.cjs) was introducedFinal Markdown Engine (
markdown.engine.mjs)The final implementation uses the following pipeline:
remark-parse– Markdown parsingremark-gfm– GitHub-flavored Markdownremark-breaks– GitHub-style line breaksremark-emoji– Emoji supportremark-rehype– Markdown → HTML ASTrehype-slug– Heading IDsrehype-external-links– External links (target="_blank")rehype-stringify– HTML outputsanitize-html– Final security sanitizationRaw HTML is not parsed into the AST, but emitted as raw HTML and sanitized afterward.
Changes Compared to Showdown
Dropped features
Image dimension syntax
This is a Showdown-specific feature with no direct replacement.
It is not standard GitHub Markdown and was removed intentionally.
Sanitization improvements
Explicit allow-list of GitHub-relevant tags:
details,summarytable,thead,tbody,tr,th,td)Explicitly allowed attributes (
id,align,class, etc.)GitHub task lists supported via disabled checkbox inputs
transformTagsensures:noopener noreferrerWhy still use
sanitize-html(and notrehype-sanitize)?While
rehype-sanitizewould integrate nicely with the AST and be slightly faster:sanitize-htmlis the de-facto industry standardAdditional Helper:
breakAfterInlineHtmlBlockThis PR also introduces a small helper function:
Why this is needed
When users paste HTML blocks directly into Markdown (for example
<details>,<table>, or headings), Markdown parsers can sometimes “swallow” the following Markdown content if it appears immediately after a closing HTML tag without a blank line.Example:
Without an explicit blank line, many Markdown parsers interpret the following content as part of the HTML block, leading to unexpected rendering.
What the function does
\n\n) after the closing tagWhy not solve this in the parser?
This behavior is part of the Markdown specification’s HTML block rules, not a bug in
remarkorrehype.Applying this normalization before parsing avoids complex AST manipulation and keeps the pipeline predictable.
Async API Changes
The new Markdown engine is asynchronous.
To handle this safely in production code:
All call sites were updated accordingly
In the config service, a lazy cache was introduced:
awaitis requiredThis matches existing behavior and can be improved separately if needed.
Conclusion
This PR modernizes Markdown handling with a secure, extensible, and well-maintained solution while:
Which issue(s) this PR fixes:
Fixes #2707
Special notes for your reviewer:
Release note: