Skip to content

mdplain: Render all text node children of link nodes #497

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SBGoods
Copy link
Contributor

@SBGoods SBGoods commented Jun 23, 2025

Description

In #484, the markdown rendering logic in mdplain package was refactored due to the deprecation of the (*BaseNode).Text() method in the yuin/goldmark library. When handling link nodes, the logic in the previous PR assumed that the text node would always be the first child of the link node. However, there are many cases where the text node is not the first child of a link node or is nested within another node, like when a code span is combined with a link:

[`link text`](http://example.com)

in the above example, the first child of the link node is the code span node and it's child is the text node containing the link text value.

This PR introduces a renderText() helper method that recursively walks all of the input node's children and writes the value of any encountered text nodes to the buffer.

Note: this does not need a changelog as the changes in #484 have not been released yet.

Rollback Plan

  • If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
No.

@SBGoods SBGoods requested a review from a team as a code owner June 23, 2025 19:25
@SBGoods SBGoods added this to the v0.22.0 milestone Jun 23, 2025
Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I ran this on the random provider and only observed one change now, which I described in the PR comment. I think it's either a bug in goldmark, or another scenario we should consider 🤔 , regardless not in scope for this PR

@@ -52,6 +52,8 @@ These are the elements outlined in John Gruber’s original design document. All

Plain URL: https://www.markdownguide.org

[`Code Link`](https://www.markdownguide.org)

Copy link
Member

@austinvalle austinvalle Jun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another change in behavior that at first glance seems unrelated to this fix, I only noticed it because of the random provider which now the only change showing is this:
image

You can actually recreate this behavior pretty easily in this test by adding:

Suggested change
[Link_With_Underscores](https://www.markdownguide.org)

The expected output would be:

Link_With_Underscores https://www.markdownguide.org

But after upgrading goldmark, we get:

Link_ https://www.markdownguide.org

Looking a little deeper it seems like the underscore is causing the link text node to get split up, which may be a bug in Goldmark? Perhaps it should be treating all link text inside those code braces as literal, maybe it thinks the link text is italicized 🤔

The workaround is pretty simple (which is probably what we would do with the random provider), you can just add backticks to the link text:

[`Link_With_Underscores`](https://www.markdownguide.org)

Will produce:

Link_With_Underscores https://www.markdownguide.org

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I do think the change might be related to this PR. It looks like each text segment delineated by _ is a sibling text node and I'm only rendering the children of the link node's first child recursively. Looping through all of the link node's children and call renderText() on each child node should fix the problem 🤞🏾

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants