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(core): ensure page.title is string, close #1306 #1308

Merged
merged 1 commit into from
May 11, 2023
Merged

Conversation

Mister-Hope
Copy link
Member

No description provided.

@Mister-Hope Mister-Hope linked an issue Apr 20, 2023 that may be closed by this pull request
@Mister-Hope Mister-Hope changed the title fix(core): ensure page.title isString, close #1306 fix(core): ensure page.title is string, close #1306 Apr 21, 2023
@@ -74,6 +74,6 @@ export const renderPageContent = ({
'frontmatter'
),
sfcBlocks,
title: frontmatter.title ?? title,
title: isString(frontmatter.title) ? frontmatter.title : title,
Copy link
Member

@meteorlxy meteorlxy Apr 24, 2023

Choose a reason for hiding this comment

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

Maybe

frontmatter.title ? `${frontmatter.title}` : title

Users should expect to use the frontmatter title if they set it, so it could be better to make something different even if the value is wrong.

Copy link
Member Author

@Mister-Hope Mister-Hope Apr 24, 2023

Choose a reason for hiding this comment

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

Emm, then why not String(frontmatter.title)

Copy link
Member

Choose a reason for hiding this comment

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

As I remember, string template has better performance than string constructor in most cases.

The only difference is that string constructor can handle symbol, while string template cannot. But I think frontmatter.title is impossible to be a symbol value in this case.

@Mister-Hope
Copy link
Member Author

This should be ready

@meteorlxy meteorlxy merged commit 644b406 into main May 11, 2023
@meteorlxy meteorlxy deleted the page-title branch May 11, 2023 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug report] Page.title is not type guarded
2 participants