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): blog should parse frontMatter.date even when time is present #5232

Merged
merged 1 commit into from
Jul 28, 2021

Conversation

wenerme
Copy link
Contributor

@wenerme wenerme commented Jul 28, 2021

Motivation

(Write your motivation here.)

frontMatter date is string, cause build error

Have you read the Contributing Guidelines on pull requests?

(Write your answer here.)

yes

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)

the problem is obvious, cause error like this https://github.com/wenerme/wener/runs/3180667805#step:5:782

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jul 28, 2021
@wenerme wenerme changed the title parse frontMatter.date fix(v2): parse frontMatter.date Jul 28, 2021
Signed-off-by: wener <wenermail@gmail.com>
@wenerme
Copy link
Contributor Author

wenerme commented Jul 28, 2021

upgrade from alpha to beta, cause build failed

@netlify
Copy link

netlify bot commented Jul 28, 2021

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 0c88816

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/6101306d165c150008d146a0

😎 Browse the preview: https://deploy-preview-5232--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Jul 28, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 62
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

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

@netlify
Copy link

netlify bot commented Jul 28, 2021

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 9f71685

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61013103486c3000071a53ca

😎 Browse the preview: https://deploy-preview-5232--docusaurus-2.netlify.app

@slorber
Copy link
Collaborator

slorber commented Jul 28, 2021

Hi, I'm not sure this fix is needed.

Can you please explain to me which frontmatter date string are you using exactly that is producing an error?
And what error did you get with it after upgrading?

I'd like to double-check that you are doing things right in the first place. The problem could also be that we don't log a good-enough error message to troubleshoot the issue.

@wenerme
Copy link
Contributor Author

wenerme commented Jul 28, 2021

@slorber Hi, you can check this build log

https://github.com/wenerme/wener/runs/3180667805#step:5:782

also this one

https://github.com/wenerme/wener/runs/3180720595#step:5:782

I give up comment out all date in front matter, and try fix the error.

This line cause the problem https://github.com/wenerme/wener/blame/master/story/2016/2016-08-06-tap-titans.md#L2

this used to works in alpha version

@slorber
Copy link
Collaborator

slorber commented Jul 28, 2021

I see thanks,

This is because Docusaurus does not really support the "blog post time".

2016-8-6 is ok, but 2016-8-6 23:43 is not.

Accepting the time may be more complicated.
We try to only parse dates as UTC and render them as UTC, because otherwise we had some tz offset issues (ie you write day 8 and it prints day 9 on your server)
See also #4881

Why do you need that time in the first place?
Docusaurus does not even display it.
Do you expect that if you write 2016-8-6 05:00 thenfor an US citizen with different TZ it would print 2016-8-5 19:00 ?

Would it be good enough if we just displayed an error telling you to remove those useless times? That seems safer to me because otherwise, we give a false impression that the time could be important and used in practice. It would be hard to use a time on a site like Docusaurus because assets are built statically. The final HTML must contain the blog post date, it can't be adjusted on a per-user basis according to visitor's tz.

@wenerme
Copy link
Contributor Author

wenerme commented Jul 28, 2021

@slorber It's about metadata, I'd like to know when I wrote the article, even it's useless to docusaurus. docusaurus can offload the tz problem to new Date(), it's depends on runtime. If docusaurus can not process the date, maybe just a warning ?

@slorber
Copy link
Collaborator

slorber commented Jul 28, 2021

Agree we could silently allow to provide a date even if it's not used in practice.

Also taking a look at the code I think it's Yaml that converts the frontmatter date of a certain pattern to a string already so it may not even apply the UTC tz when doing so.

I think current system is not perfect and has some edge cases and your PR won't break it more, so going to merge this for now

@slorber slorber changed the title fix(v2): parse frontMatter.date fix(v2): blog should parse frontMatter.date even when time is present Jul 28, 2021
@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Jul 28, 2021
@slorber slorber merged commit bb0c9ee into facebook:master Jul 28, 2021
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.

3 participants