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(v2): fix too strict markdown frontmatter validation #4654

Merged
merged 8 commits into from
Apr 21, 2021
Merged

fix(v2): fix too strict markdown frontmatter validation #4654

merged 8 commits into from
Apr 21, 2021

Conversation

johnnyreilly
Copy link
Contributor

@johnnyreilly johnnyreilly commented Apr 21, 2021

Motivation

PR completed by @slorber:


Original description:

To either get a meaningful error message when invalid tags are supplied, or allow them to be parsed successfully (numbers are not) - this relates to #4642

Hey @slorber,

I've started work on this and have some questions. Whilst I can get meaningful error messages using the mechanism you suggest (see changes in blogFrontMatter.ts) the suggested:

    Joi.attempt(frontMatter, BlogFrontMatterSchema, {convert: true});

does not seem to support parsing numbers to strings automatically as suggested. Also, even if it did, it won't actually change the type of the tags in the frontMatter object. It's just an assertion - no mutation takes place. We could put something in the markdownParser here:

https://github.com/facebook/docusaurus/blob/master/packages/docusaurus-utils/src/markdownParser.ts#L68-L79

that migrates numbers to strings.... Or alternatively, maybe it just throwing a meaningful error message is fine. I think the meaningful error message is the most significant thing from my perspective.

What do you think?

Have you read the [Contributing Guidelines on pull requests]

Yes

Test Plan

Automated tests!

Related PRs

N/A

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 21, 2021
@netlify
Copy link

netlify bot commented Apr 21, 2021

[V2]

Built with commit fbfa345

https://deploy-preview-4654--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Apr 21, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 61
🟢 Accessibility 96
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-4654--docusaurus-2.netlify.app/

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Thanks @johnnyreilly , I see the problem

I'd like to do a small bugfix release soon, and have a few issues reported regarding this frontmatter validation.

As this is likely to create conflicts (like with #4655), I'll take your PR as a base and complete it so that I can release the fix asap!

I changed the assertion to actually return the value. I will try to see if I can convert to number from there

.devcontainer/devcontainer.json Show resolved Hide resolved
@netlify
Copy link

netlify bot commented Apr 21, 2021

[V1]

Built with commit fbfa345

https://deploy-preview-4654--docusaurus-1.netlify.app

@slorber slorber changed the title WIP: tag parsing fix #4642 fix(v2): fix too strict markdown frontmatter validation Apr 21, 2021
@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Apr 21, 2021
@slorber slorber marked this pull request as ready for review April 21, 2021 14:04
@slorber slorber requested a review from lex111 as a code owner April 21, 2021 14:04
@slorber slorber merged commit e11597a into facebook:master Apr 21, 2021
@slorber slorber linked an issue Apr 21, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2.0.0-alpha.73 can't handle tags which could be parsed as numbers
3 participants