Skip to content
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

Encode ampersands in markdown code blocks #3630

Merged
merged 5 commits into from
Jun 20, 2022
Merged

Encode ampersands in markdown code blocks #3630

merged 5 commits into from
Jun 20, 2022

Conversation

tony-sull
Copy link
Contributor

Closes #3487

Changes

Updates rehype to encode ampersands in code blocks

Testing

Added tests for markdown <code> and fenced code blocks

Docs

N/A

@changeset-bot
Copy link

changeset-bot bot commented Jun 17, 2022

🦋 Changeset detected

Latest commit: afaab05

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@astrojs/markdown-remark Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the feat: markdown Related to Markdown (scope) label Jun 17, 2022
@tony-sull tony-sull marked this pull request as ready for review June 17, 2022 20:40
Copy link
Member

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

LGTM! A shame we have to do all of these as one-offs. For a future PR, is there a more generic encoding function or html encoder npm package that we could call on this? No worries if not, just a thought.

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

LGTM!

@tony-sull
Copy link
Contributor Author

@FredKSchott I believe @hippotastic is actually working on a larger update that moves more of the work out of our custom markdown plugins - hopefully the days of one-off markdown fixes are coming to an end!

@tony-sull tony-sull merged commit 48e67fe into main Jun 20, 2022
@tony-sull tony-sull deleted the fix/md-ampersands branch June 20, 2022 19:09
@github-actions github-actions bot mentioned this pull request Jun 20, 2022
@hippotastic
Copy link
Contributor

hippotastic commented Jun 20, 2022

Hey, thanks for mentioning that I'm working on something in that area! 😄 Just to clarify a bit - I'm working on two main things right now:

  1. A large amount of new test cases which ensure that our Markdown-to-HTML pipeline works as it should, and doesn't crash or produce invalid output. I didn't trust our test coverage so far, so I always ran a build of our Astro Docs site and performed a side-by-side file comparison of the build output between Astro versions to find any issues introduced by Markdown-related code changes. This is how I also found this bug that Tony just fixed. This is of course not something we can expect other contributors to do, and our tests should cover that.
  2. A preprocessor that is able to transform documents that are valid Markdown as per the CommonMark/GFM spec to valid MDX documents that (A) do not crash the parser because of trivial formatting mistakes and (B) produce HTML output that comes as close as possible to the Markdown author's recognizable intention. This is important to make migration to Astro as smooth as possible - if you look at a document and think that it looks valid, it should just work and not fail the build due to a seemingly unclosed nested paragraph.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: markdown Related to Markdown (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 BUG: Ampersands (&) contained in Markdown are not HTML-encoded
4 participants