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

Unset charset=utf-8 content-type for md/mdx pages #12231

Merged
merged 10 commits into from
Oct 24, 2024
Merged

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Oct 15, 2024

Changes

charset=utf-8 will no longer be added to Content-Type in markdown and mdx pages. This matches the behaviour of other pages, e.g. astro or html pages.

This PR

This PR adds <meta charset="utf-8"> automatically to all md/mdx pages if they have no layout. This allows users to type non-ASCII characters without setting up a layout to add the meta tag into the head.

Would like your thoughts

A revelation I had (which is very obvious in hindsight) is that you could easily add <meta charset="utf-8" /> to the .md or .mdx pages yourself directly. They accept HTML anyways and you don't really need a layout to put the meta tag in the head to work. So this whole PR felt causing more hidden magic?

Do you think it's worth having the behaviour described in the This PR section above? Or simply remove it so that users should add the meta tag themselves?

Testing

We have existing tests that covers this behaviour.

Docs

withastro/docs#9757

Is somewhat a breaking change which will be documented in the migration guide later. We may also want to add a charset note in https://docs.astro.build/en/basics/layouts/#markdown-layouts.

Copy link

changeset-bot bot commented Oct 15, 2024

🦋 Changeset detected

Latest commit: 6348b48

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 pkg: integration Related to any renderer integration (scope) pkg: astro Related to the core `astro` package (scope) semver: major Change triggers a `major` release labels Oct 15, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a major changeset. A reviewer will merge this at the next release if approved.

@ematipico
Copy link
Member

ematipico commented Oct 15, 2024

I personally think that users should provide the tag themselves. However, Astro is the content framework, so the framework should help users display any type of content, even when it isn't UTF-8.

If our head propagation is solid, the I am in favour of This PR, and then users can add the meta tag themselves if they need to change it.

@bluwy
Copy link
Member Author

bluwy commented Oct 16, 2024

If our head propagation is solid, the I am in favour of This PR, and then users can add the meta tag themselves if they need to change it.

For markdown or MDX pages without any layout, we actually send the generated HTML as is. So a # Heading is simply <h1>Heading</h1> and we sent that (plus doctype) as is. No <html>, <head>, <body> or anything, which is fine for browsers and also means that there isn't any head propagation involved in this case.

If the user were to add the <meta> tag manually, it'd actually achieve the same output as this PR. This PR automates the part as if the user has added the code manually in practice.

Anyways, wanted to clarify a little 😅 I do think there's a good point where Astro is content-focused and it may good to have this automatic behaviour ootb.

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Looks good to me

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.

Thanks @bluwy ! Left some comments and a question because I'm not sure whether MDX is going to work exactly the same, or slightly differently. (Since with MDX, there are two different ways to "use a layout" vs only the one way for Markdown pages.)

.changeset/strong-months-grab.md Outdated Show resolved Hide resolved
.changeset/strong-months-grab.md Outdated Show resolved Hide resolved
.changeset/strong-months-grab.md Outdated Show resolved Hide resolved
.changeset/dirty-cooks-explode.md Show resolved Hide resolved
.changeset/dirty-cooks-explode.md Outdated Show resolved Hide resolved
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.

Approving for docs! I still have a tiny bit of work to do on my end to incorporate this on a couple of different pages, but nothing blocking release! 🙌

@bluwy bluwy merged commit 90ae100 into next Oct 24, 2024
2 checks passed
@bluwy bluwy deleted the plt-1057-charset-utf-8 branch October 24, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope) semver: major Change triggers a `major` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants