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

fix(markdown): don’t generate mdast html nodes #10104

Merged
merged 13 commits into from
Mar 8, 2024

Conversation

remcohaszing
Copy link
Contributor

Changes

html nodes from mdast are converted to raw hast nodes. These nodes are then not processed by proper rehype plugins. Typically if a remark plugin generates html nodes, this indicates it should have actually been a rehype plugin.

This changes the remark plugins that generate html nodes into rehype nodes. These were remarkPrism and remarkShiki.

Testing

pnpm build -- --force
pnpm test

No tests were added, because it mostly changes internals.

Docs

This affects a user’s behaviour by adding better compatibility with some existing remark / rehype plugins. E.g. rehype-mermaid will work. I don’t think a docs update is needed.

Closes #9909

`html` nodes from mdast are converted to `raw` hast nodes. These nodes
are then not processed by proper rehype plugins. Typically if a remark
plugin generates `html` nodes, this indicates it should have actually
been a rehype plugin.

This changes the remark plugins that generate `html` nodes into rehype
nodes. These were `remarkPrism` and `remarkShiki`.

Closes withastro#9909
Copy link

changeset-bot bot commented Feb 13, 2024

🦋 Changeset detected

Latest commit: 06bd1e3

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

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 feat: markdown Related to Markdown (scope) pkg: integration Related to any renderer integration (scope) labels Feb 13, 2024
@bluwy
Copy link
Member

bluwy commented Feb 19, 2024

The code looks good to me, but isn't this a breaking change as the order is changed, and code is transformed differently? Or do you foresee that it shouldn't break much?

@remcohaszing
Copy link
Contributor Author

I see why this can be considered a breaking change. On the other hand, it only really breaks if people rely on the bad AST that Astro produced earlier. In that case I would say it breaks people’s workarounds.

I don’t expect this to break people’s code, but I’m not sure how people are using this. I do know this fixes compatibility with rehype-mermaid and potentially other rehype plugins.

Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

I have the same concerns as Bjorn, but I do think it's okay!

@ematipico
Copy link
Member

As middle ground, we could release these changes as minor and add a warning for a possible breaking change if integrations were relying on broken AST. What do you think @bluwy and @Princesseuh ?

@Princesseuh
Copy link
Member

As middle ground, we could release these changes as minor and add a warning for a possible breaking change if integrations were relying on broken AST.

Fine with me! We'll mention it in the article.

@ematipico ematipico added this to the 4.5 milestone Feb 19, 2024
@ematipico ematipico added the semver: minor Change triggers a `minor` release label Feb 19, 2024
@remcohaszing
Copy link
Contributor Author

Don't forget about this comment: #10104 (comment). Whatever you choose is fine, just make it explicit :)

@bluwy
Copy link
Member

bluwy commented Feb 19, 2024

Marking as minor is fine by me too 👍 Let's release it in 4.5 then.

@OliverSpeir
Copy link
Contributor

Having both before user rehype plugins I think is important otherwise great change I know rehype mermaid has been requested

@github-actions github-actions bot added the pkg: create-astro Related to the `create-astro` package (scope) label Feb 22, 2024
@github-actions github-actions bot removed the pkg: create-astro Related to the `create-astro` package (scope) label Feb 22, 2024
@ematipico
Copy link
Member

It would be great if we could craft a good changelog explaining the possible breaking change. @remcohaszing would you mind doing that? cc @bluwy might help, he has more context on the matter.

@remcohaszing
Copy link
Contributor Author

Do you mean that as part of the changeset?

@ematipico
Copy link
Member

Yeah sorry. That's what I meant

This also includes some hints on what users could do to upgrade of they
rely on these nodes.
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Thanks for following up with the requested changes! Appreciate the refactor and cleanup.

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution @remcohaszing -- seems like this will ensure some greater compatibility, and we at Astro love our code samples!

I've made some small suggestions below because this is not only a change, but potentially a breaking change, and this changeset message is a great way to call attention to that and provide the most warning, guidance and support to people who might need to make a change because of this.

See what you think and correct to be accurate if I've lost any of the particular meaning by editing! 🙌

.changeset/thirty-beds-smoke.md Outdated Show resolved Hide resolved
.changeset/thirty-beds-smoke.md Outdated Show resolved Hide resolved
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Docs is approving the changeset, as long as everyone else is happy with it!

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@ematipico ematipico merged commit a31bbd7 into withastro:main Mar 8, 2024
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Mar 8, 2024
@remcohaszing remcohaszing deleted the fix-rehype-syntax-highlighting branch March 8, 2024 11:19
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) pkg: integration Related to any renderer integration (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Astro wrongly process markdown code
6 participants